To inline or not to inline

I've been writing a few classes lately; and I was wondering whether it's bad practice, bad for performance, breaks encapsulation or whether there's anything else inherently bad with actually defining some of the smaller member functions inside a header (I did try Google!). Here's an example I have of a header I've written with a lot of this:

class Scheduler {
public:
    typedef std::list<BSubsystem*> SubsystemList;

    // Make sure the pointer to entityManager is zero on init
    // so that we can check if one has been attached in Tick()
    Scheduler() : entityManager(0) { }

    // Attaches a manager to the scheduler - used by Tick()
    void AttachEntityManager( EntityManager &em )
        { entityManager = &em; }

    // Detaches the entityManager from a scheduler.
    void DetachEntityManager()
        { entityManager = 0; }

    // Adds a subsystem to the scheduler; ex开发者_开发技巧ecuted on Tick()
    void AddSubsystem( BSubsystem* s )
        { subsystemList.push_back(s); }

    // Removes the subsystem of a type given
    void RemoveSubsystem( const SubsystemTypeID& );

    // Executes all subsystems
    void Tick();

    // Destroys subsystems that are in subsystemList
    virtual ~Scheduler();
private:
    // Holds a list of all subsystems
    SubsystemList subsystemList;

    // Holds the entity manager (if attached)
    EntityManager *entityManager;
};

So, is there anything that's really wrong with inlining functions like this, or is it acceptable?

(Also, I'm not sure if this'd be more suited towards the 'code review' site)

---

Inlining increases coupling, and increases "noise" in the class definition, making the class harder to read and understand. As a general rule, inlining should be considered as an optimization measure, and only used when the profiler says it's necessary.

There are a few exceptions: I'll always inline the virtual destructor of an abstract base class if all of the other functions are pure virtual; it seems silly to have a separate source file just for an empty destructor, and if all of the other functions are pure virtual, and there are no data members, the destructor isn't going to change without something else changing. And I'll occasionally provide inlined constructors for "structures"—classes in which all data members are public, and there are no other functions. I'm also less rigorous about avoiding inline in classes which are defined in a source file, rather than a header—the coupling issues obviously don't apply in that case.

---

All of your member functions are one-liners, so in my opinion thats acceptable. Note that inline functions may actually decrease code size (!!) because optimizing compilers increase the size of (non-inline) functions in order to make them fit into blocks.

In order to make your code more readable I would suggest to use inline definitions as follows:

class Scheduler
{
    ...

    void Scheduler::DetachEntityManager();

    ...
};


inline void Scheduler::DetachEntityManager()
{
    entityManager = 0;
}

In my opinion thats more readable.

---

I think inlining (if I understood you right, you mean the habit of writing trivial code right into the header file, and not the compiler behaviour) aids readability by two factors:

1. It distinguishes trivial methods from non-trivial ones.
2. It makes the effect of trivial methods available at a glance, being self-documenting code.

From a design POV, it doesn't really matter. You are not going to change your inlined method without changing the subsystemList member, and a recompile is necessary in both cases. Inlining does not affect encapsulation, since the method is still a method with a public interface.

So, if the method is a dumb one-liner without a need for lengthy documentation or a conceivable need of change that does not encompass an interface change, I'd advise to go for inlining.

---

It will increase executable size and in some occasions this will lead to worse performance.

Keep in mind that an inline method requires it's source code to be visible to whoever uses it (ie. code in the header) this means that a small change in the implementation of your inlined methods will cause a recompilation on everything that uses the header where the inline method was defined.

On the other hand, it is a small performance increase, it's good for short methods that are called really frequently, since it will save you the typical overhead of calling to methods.

Inline methods are fine if you know where to use them and don't spam them.

Edit: Regarding style and encapsulation, using inline methods prevents you from using things like Pointer to implementation, forward declarations, etc.. since your code is in the header.

---

Inlining has three "drawbacks" at least:

1. inline functions are at odds with the virtual keyword (I mean conceptually, IMO, either you want a piece of code to be substituted for the function call, or you want the function call to be virtual, i.e. polymorphic; anyway, see also this for more details as to when it could make sense practically);

2. your binary code will be larger;

3. if you include the inline method in the class definition, you reveal implementation detail.

Apart from that it is plainly ok to inline methods, although it is also true that modern compilers are already sufficiently smart to inline methods on their own when it makes sense for performance. So, in a sense I think it is better to leave it to the compiler altogether...

---

Methods inside class body are usually inline automatically. Also, inline is a suggestion and not a command. Compilers are generally smart enough to judge whether to inline a function or not.

You can refer to this similar question.

---

In fact you can write all your functions in the header file, if the function is too large the compiler will automatically not inline the function. Just write the function body where you think it fits best, let the compiler decide. The inline keyword is ignored often as well, if you really insist on inlining the function use __forceinline or something similar (I think that is MS specific).