C++: Creating a shared object rather than a shared pointer to an object

Question

boost::shared_ptr really bothers me. Certainly, I understand the utility of such a thing, but I wish that I could use the shared_ptr<A> as an A*. Consider the following code

class A
{
public:
    A() {}
    A(int x) {mX = x;}
    virtual void setX(int x) {mX = x;}
    virtual int getX() const {return mX;}
private:
    int mX;
};


class HelpfulContainer
{
public:
    //Don't worry, I'll manager the memory from here.
    void eventHorizon(A*& a)
    {
        cout << "It's too late to save it now!" << endl;
        delete a;
        a = NULL;
    }
};


int main()
{
    HelpfulContainer helpfulContainer;

    A* a1 = new A(1);
    A* a2 = new A(*a1);
    cout << "*a1 = " << *a1 << endl;
    cout << "*a2 = " << *a2 << endl;
    a2->setX(2);
    cout << "*a1 = " << *a1 << endl;
    cout << "*a2 = " << *a2 << endl;
    cout << "Demonstrated here a2 is not connected to a1." << endl;

    //hey, I wonder what this event horizon function is.
    helpfulContainer.eventHorizon(a1);

    cout << "*a1 = " << *a1 << endl;//Bad things happen when running this line.
}

Whoever created the HelpfulContainer wasn't thinking about others wanting to retain pointers to A objects. We can't give HelpfulClass boost::shared_ptr objects. But one thing we could do is use the pimlp idiom to create a SharedA which itself is an A:

class SharedA : public A
{
public:
    SharedA(A* a) : mImpl(a){}
    virtual void setX(int x) {mImpl->setX(x);}
    virtual int getX() const {return mImpl->getX();}
private:
    boost::shared_ptr<A> mImpl;
};

And then the main function can look something like this:

int main()
{
    HelpfulContainer helpfulContainer;

    A* sa1 = new SharedA(new A(1));
    A* sa2 = new SharedA(sa1);
    cout << "*sa1 = " << *sa1 << endl;
    cout << "*sa2 = " << *sa2 << endl;
    sa2->setX(2);
    cout << "*sa1 = " << *sa1 << endl;
    cout << "*sa2 = " << *sa2 << endl;
    cout << "this demonstrates that sa2 is a shared version of sa1" << endl;

    helpfulContainer.eventHorizon(sa1);
    sa2->setX(3);
    //cout << "*sa1 = " << *sa1 << endl;//Bad things would happen here
    cout << "*sa2 = " << *sa2 << endl; 
    //but this line indicates that the originally created A is still safe and intact.
    //only when we call sa2 goes out of scope will the A be deleted.
}

So, my question is this: Is the above pattern a good pattern, or is there something I'm not considering yet. My current project inherited a HelpfulContainer class like above that's deleting the pointers that I need, but I still need the data structure present in the HelpfulContainer.

Update: This question is a follow-on question.

If HelpfulContainer wants to take ownership of the pointer it should use the correct semantics to do so. The interface `eventHorizon` is badly named as it does not explain what is happening and the parameter it takes should indicate that ownership is being transfered (say std::auto_ptr or its new replacement std::unique_ptr). Both of these would indicate that the HelpfullContainer object is taking ownership of the object and thus it will no longer be valid after the call. So you prove the point that in C++ code can be written badly by people who do not understand the semantics of the language. — Martin York, Dec 10 '10 at 20:49

score 6 · Accepted Answer · answered Dec 10 '10 at 17:03

The whole point of shared_ptr is that it (and its copies) own the object that it points to. If you want to give an A to a container that manages its lifetime then you shouldn't be using a shared_ptr at all as it doesn't meet your needs; HelpfulContainer only knows how to be the sole owner of a dynamically created object so you need to give it a pointer to an object that isn't owned by anything else.

I think that it is usually poor design for an object to care about its own lifetime (there are exceptions). It is usually more useful if an object can do a job and something else manages its creation and descruction, choosing the simplest lifetime strategy possible (e.g. local/automatic variable).

If you absolutely have to share ownership between two things that don't co-operate (such as shared_ptr and HelpfulContainer) then you will have to use some sort of proxy technique.

In this case, though, it just looks like HelpfulContainer just isn't that helpful for your situation.

+1: just change that damn container (and carefully review the code using it), adding an overload taking a `shared_ptr` could be useful, for example. — Matthieu M., Dec 10 '10 at 18:15

score 1 · Answer 2 · answered Dec 10 '10 at 17:09

1

I'm not sure what this does for you.

If helpfulContainer.eventHorizon() always deletes its parameter, then why not just pass a new copy of (the original) A class:

  helpfulContainer.eventHorizon(new A(sa1));

Or, if helpfulContainer.eventHorizon() only sometimes deletes its parameter, then making the call as

  helpfulContainer.eventHorizon(new SharedA(sa1));

will leak both the SharedA and the original A (sa1) on those occasions when it chooses not to delete.

answered Dec 10 '10 at 17:09

Dan Breslau

11,472
2
35
44

What of the case that the eventHorizon doesn't delete the pointer, but stores it in a data structure some where. If the associated shared_ptrs are all deleted, then there is an invalid pointer hiding in the data structure. – JnBrymn Dec 10 '10 at 17:46
If you know that eventHorizon is going to take ownership of the pointer, and will delete it eventually, and you need to keep ownership for yourself -- then pass a new copy, as in the first form that I showed (`helpfulContainer.eventHorizon(new A(sa1));`). – Dan Breslau Dec 10 '10 at 17:50
@John Berryman: Maybe I misinterpreted your comment. To your first sentence: If eventHorizon holds the pointer, and deletes it at its leisure, and you still need access to that same object, then your proposal **might** help. But you would need to very thoroughly refactor your code so that pointers to raw `A` objects are never used except via a `SharedA` object. Any attempt to mix the two would lead to much wailing and gnashing of teeth. I think you'll also want a specialized copy constructor for SharedA(const SharedA&) that copies `mImpl`. – Dan Breslau Dec 10 '10 at 18:10
@John Berryman: To your second sentence: I don't understand what you mean here. If, truly, **all** of the shared_ptrs are deleted (inside and outside of the container) -- and I'm assuming you meant "destroyed" rather than "deleted", since boost::shared_ptr is an object -- then you don't have any invalid pointers anywhere. Unless you mixed raw and shared pointers to the same object... but you don't want to do that, ever. – Dan Breslau Dec 10 '10 at 18:13

score 0 · Answer 3 · answered Dec 10 '10 at 16:23

So you are creating a Stand-In (SharedA) for which deletion is okay. Even though this is kinda awkward, I guess it's necessary to work with your legacy API. To slightly improve this: Allow construction of SharedA from a shared_ptr, but not the other way around - and then only use the SharedP when you absolutely must:

int main()
{
  HelpfulContainer helpfulContainer;

  boost::shared_ptr<A> sa1(new A(1));

  // deletes its parameter, but that's okay
  helpfulContainer.eventHorizon(new SharedA(sa1)); 
}

score 0 · Answer 4 · answered Dec 10 '10 at 18:31

Implicit conversions to the underlying pointer type are inconsistent with the intended use of shared_ptr in that you can extremely easily pass the shared_ptr to a function etc without realizing it.

It sounds to me like HelpfulContainer is anything BUT helpful and should be fixed or ditched.

If that's not possible then probably the best way is to just copy the A you want to pass in and pass the copy to the container.

C++: Creating a shared object rather than a shared pointer to an object

4 Answers4

Linked