To inline or not to inline

Question

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; executed 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)

Answer 1

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.

Answer 2

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.

Answer 3

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.