safe enable_shared_from_this usage

Question

Please consider the following simplified example:

#include "boost/shared_ptr.hpp"
#include "boost/smart_ptr/enable_shared_from_this.hpp"

using namespace boost;

class State : public boost::enable_shared_from_this<State>
{
public:
    shared_ptr<State> GetSelf()
    {
        return shared_from_this();
    };
    // returns following state
    virtual shared_ptr<State> Execute() = 0;
};

template<typename T>
shared_ptr<T> Create()
{
    return shared_ptr<T>(new T);
}

class MyState2;

class MyState1 : public State
{
    virtual shared_ptr<State> Execute()
    {
            if (change of state)
          return Create<MyState2>();
            else
              return GetSelf();
    };
};

class MyState2 : public State
{
    virtual shared_ptr<State> Execute()
    {
            if (change of state)
          return Create<MyState1>();
            else
              return GetSelf();
    }; 
};

int main(int argc, char* argv[])
{
    shared_ptr<State> pCurrentState = Create<MyState1>();  // <-- OK
//  State* pCurrentState(new MyState1);                    // <-- Runtime error
    while (...)
      pCurrentState = pCurrentState->Execute();

    return 0;
}

The State-class is part of a 'framework'. It is the base for user defined states in a lightweight state machine, where each state returns its follower state. In case the state does not change, it returns itself. The framework user may arbitrarily derrive from class State. Obviously it is not allowed to create 'uncontrolled' instances of derrived classes (would result in runtime error if GetSelf() is invoked). To point the user to the right direction, a Create() function template is provided to create 'controlled' instances. Since I want to make usage as foolproof as possible, how can I make sure a user won't create any instances of derrived states which are not under shared_ptr control? Or at least give a meaningful error message (at compile time?) if he does so.

Thanks for your help!

Answer 1

The simplest way to prevent unmanaged instances is to make the constructor of each derived class private. Your factory function will need to be either a friend or a member of the class it's creating; if you want to keep your factory template, then it will need to be declared before the derived classes in order to make it a friend:

template<typename T>
shared_ptr<T> Create()
{
    return shared_ptr<T>(new T);
}

class Derrived : public Base
{
private:
    friend shared_ptr<Derrived> Create<Derrived>();
    Derrived() {}
    // Client implementation goes here
};

However, I would usually recommend that you don't force a class to be managed in a particular way. You should only do something like this if the classes themselves require that they are managed by shared pointers in order to function correctly (that is, they need to call shared_from_this() from their member functions for some reason). Otherwise, just organise your framework so that it uses shared pointers, and anyone using the framework will have to do likewise with no need for strange enforcement mechanisms. There should be no need for GetSelf() at all, since you'll always have a shared pointer available to copy if you have access to an object at all, and no need for enable_shared_from_this unless the classes need to access shared pointers internally (which is rather an unusual thing to do).

UPDATE: In your use case, I don't think it's possible for the base class to enforce the use of shared pointers (but please correct me if I'm wrong). It might make sense to enforce the use of shared pointers when calling the functions that require them. You'll be able to create unmanaged objects, but not do anything dangerous with them.

class State : public boost::enable_shared_from_this<State>
{
public:
    // returns following state
    friend shared_ptr<State> Execute(shared_ptr<State> state) {
        return state->Execute();
    }
private:
    virtual shared_ptr<State> Execute() = 0;
};

class MyState : public State
{
    shared_ptr<State> Execute() {return shared_from_this();}
};

int main(int argc, char* argv[])
{
    shared_ptr<State> state1(new MyState);  // <-- OK
    State * state2(new MyState);            // <-- OK, but can't be used
    Execute(state1);                        // <-- OK
    // Execute(state2);                     // <-- Error
    // state2->Execute();                   // <-- Error
}

Answer 2

There is no way to do what you want in the general case.

If Base isn't a base class (ie: they're not supposed to derive from it), then it's easy. Make your constructor(s) factory function(s) and make the constructor(s) private. Therefore, they have to go through your factories, and therefore you can ensure that they used shared_ptr to wrap them.

However, if you are allowing the user to derive classes from them, then all bets are off. So you need to think about two things:

1. Do I really need enable_shared_from_this here?
2. Do I really need to allow inheritance?

1: The code you posted does not really give a reason why you need to use enable_shared_from_this at all. The only place you use it is in a function that returns a shared_ptr to that instance. Well, who is it returning it to? The user of that object. Who can only create an instance... by having a shared_ptr.

In short: you give them something that they necessarily already have access to. The function is useless.

Well, it's useless unless you are violating proper use of shared_ptr. In general, once you wrap a pointer in a shared_ptr, you either keep it that way or make a weak_ptr out of it. You almost never give someone a naked pointer to that object. And if you do, then it should be a clear sign that they are not to store that pointer or expect to maintain any form of ownership of it in any way, shape, or form.

Therefore, them being able to get a shared_ptr from a naked pointer is bad design.

enable_shared_from_this is intended for the single use of naked pointers to shared_ptr held objects that you cannot avoid: this. It exists because a member function might need to call an external function with a pointer to that object, but that external function takes a shared_ptr or a weak_ptr instead of a naked pointer. It is not meant to be a way to allow bad design.

2: You're building a state machine. So why does the base class State need to be derived from enable_shared_from_this? If a derived class's Execute method is going to return itself, that's up to that derived class to deal with it. It should not be the responsibility of the base class to provide that. And it certainly should not be provided by a public function.

In short, each derived class needs to inherit from enable_shared_from_this if it wants to return a pointer to itself. It therefore becomes the user's responsibility to keep these things straight, not yours.