views:

2592

answers:

8

Just wanted opinions on a design question. If you have a C++ class than owns other objects, would you use smart pointers to achieve this?

class Example {
public: 
  // ...

private:
  boost::scoped_ptr<Owned> data;
};

The 'Owned' object can't be stored by value because it may change through the lifetime of the object.

My view of it is that on the one side, you make it clear that the object is owned and ensure its deletion, but on the flipside, you could easily just have a regular pointer and delete it in the destructor. Is this overkill?

Follow up: Just wanted to say thanks for all your answers. Thanks for the heads-up about auto_ptr leaving the other object with a NULL pointer when the whole object is copied, I have used auto_ptr extensively but had not thought of that yet. I make basically all my classes boost::noncopyable unless I have a good reason, so there's nothing to worry about there. And thanks also for the information on memory leaks in exceptions, that's good to know too. I try not to write things which could cause exceptions in the constructor anyway - there are better ways of doing that - so that shouldn't be a problem.

I just had another question though. What I wanted when I asked this question was to know whether anyone actually did this, and you all seem to mention that it's a good idea theoretically, but no one's said they actually do it. Which surprises me! Certainly one object owning a pointer to another is not a new idea, I would have expected you all would have done it before at some point. What's going on?

+6  A: 

It's not overkill at all, it's a good idea.

It does require your class clients to know about boost, though. This may or may not be an issue. For portability you could consider std::auto_ptr which does (in this case) the same job. As it's private, you don't have to worry about other people attempting to copy it.

Charles Bailey
Unfortunately using std::auto_ptr instead of boost::scoped_ptr doesn't do the same, so the disadvantage is that you need to know scoped_ptr. scoped_ptr prevents copying of the object where it is in, whereas auto_ptr does not (which can lead to strange and hard to find bugs).
vividos
I said: "in this case" and "does the same job". I didn't claim that they were identical. In this case, as the member is private, copying by client code is prevented, only functions in the class itself have the chance to be 'incorrect'.
Charles Bailey
Charles, vividos is right i think. You can just do Example e, a; e = a; and a will be left with a resetted auto_ptr.so you at least should code a copy constructor and don't copy it.Another point is you need to define ur own dtor in the .cpp file,and you can't have Owned be incomplete in your .h file
Johannes Schaub - litb
so yeah there are a couple of pitfalls when using auto_ptr for this job. while using an incomplete type often works, it's UB in theory. You can often get away with defining the dtor of your class in the .cpp file, making the auto_ptr dtor not doing the delete inline inside the .h file .
Johannes Schaub - litb
Holding a resource via a raw pointer means that you *have* to provide (or suppress) custom copy and assign operations. I took this as read. Changing to std::auto_ptr doesn't change this, but changing to scoped_ptr gets you suppression for free.
Charles Bailey
You are right that the standard does not allow the template parameter for auto_ptr to be a incomplete type even if you are usually OK if you define the type before causing reset, the copy or assignment operators, or the destructor to be instantiated.
Charles Bailey
+4  A: 

Scoped pointers are good for exactly this because they ensure that the objects get deleted without you having to worry about it as a programmer. I think that this is a good use of scoped ptr.

I find that a good design strategy in general is to avoid freeing memory manually as much as possible and let your tools (in this case smart pointers) do it for you. Manual deletion is bad for one main reason as I can see it, and that is that code becomes difficult to maintain very quickly. The logic for allocation and deallocation of memory is often separate in the code, and that leads to the complementary lines not being maintained together.

Daniel Nadasi
+3  A: 

I don't think this is overkill, this documents the semantics of the member much better than having a raw pointer and is less error prone.

Motti
+9  A: 

It's a good idea. It helps simplify your code, and ensure that when you do change the Owned object during the lifetime of the object, the previous one gets destroyed properly.

You have to remember that scoped_ptr is noncopyable, though, which makes your class noncopyable by default until/unless you add your own copy constructor, etc. (Of course, using the default copy constructor in the case of raw pointers would be a no-no too!)

If your class has more than one pointer field, then use of scoped_ptr actually improves exception safety in one case:

class C
{
  Owned * o1;
  Owned * o2;

public:
  C() : o1(new Owned), o2(new Owned) {}
  ~C() { delete o1; delete o2;}
};

Now, imagine that during construction of a C the second "new Owned" throws an exception (out-of-memory, for example). o1 will be leaked, because C::~C() (the destructor) won't get called, because the object has not been completely constructed yet. The destructor of any completely constructed member field does get called though. So, using a scoped_ptr instead of a plain pointer will allow o1 to be properly destroyed.

SCFrench
+3  A: 

Using the scoped_ptr is a good idea.

Keeping and manually destroying the pointer is not as simple as you think. Especially if there is more than one RAW pointer in your code. If exception safety and not leaking memory is a priority then you need a lot of extra code to get it correct.

For a start you have to make sure you correctly define all four default methods. This is because the compiler generated version of these methods are fine for normal objects (including smart pointers) but in the normal case will lead to problems with pointer handling (Look for the Shallow Copy Problem).

  • Default Constructor
  • Copy Constructor
  • Assignment Operator
  • Destructor

If you use the scoped_ptr then you don't need to worry about any of those.

Now if you have more than one RAW pointer in your class (or other parts of your constructor can throw) . You have to EXPLICITLY deal with exceptions during construction and destruction.

class MyClass
{
    public:
        MyClass();
        MyClass(MyClass const& copy);
        MyClass& operator=(MyClass const& copy);
        ~MyClass();

    private
        Data*    d1;
        Data*    d2;
};

MyClass::MyClass()
    :d1(NULL),d2(NULL)
{
    // This is the most trivial case I can think off
    // But even it looks ugly. Remember the destructor is NOT called
    // unless the constructor completes (without exceptions) but if an
    // exception is thrown then all fully constructed object will be
    // destroyed via there destructor. But pointers don't have destructors.
    try
    {
        d1 = new Data;
        d2 = new Data;
    }
    catch(...)
    {
        delete d1;
        delete d2;
        throw;
    }
}

Look how much easier a scopted_ptr is.

Martin York
+1, and this is without even the reference count twiddling! (You've got copy constructors there, not shown here).
Arafangion
+2  A: 

Why an overkill? boost::scoped_ptr is very easy to optimize, and I bet the resulting machine code would be the same as if you manually delete the pointer in the destructor.

scoped_ptr is good - just use it :)

Nemanja Trifunovic
+10  A: 

scoped_ptr is very good for this purpose. But one has to understand its semantics. You can group smart pointers using two major properties:

  • Copyable: A smart pointer can be copied: The copy and the original share ownership.
  • Movable: A smart pointer can be moved: The move-result will have ownership, the original won't own anymore.

That's rather common terminology. For smart pointers, there is a specific terminology which better marks those properties:

  • Transfer of Ownership: A smart pointer is Movable
  • Share of Ownership: A smart pointer is copyable. If a smart pointer is already copyable, it's easy to support transfer-of-ownership semantic: That then is just an atomic copy & reset-of-original operation, restricting that to smart pointers of certain kinds (e.g only temporary smart pointers).

Let's group the available smart pointers, using (C)opyable, and (M)ovable, (N)either:

  1. boost::scoped_ptr: N
  2. std::auto_ptr: M
  3. boost::shared_ptr: C

auto_ptr has one big problem, in that it realizes the Movable concept using a copy constructor. That is because When auto_ptr was accepted into C++, there wasn't yet a way to natively support move semantics using a move constructor, as opposed to the new C++ Standard. That is, you can do the following with auto_ptr, and it works:

auto_ptr<int> a(new int), b;
// oops, after this, a is reset. But a copy was desired!
// it does the copy&reset-of-original, but it's not restricted to only temporary
// auto_ptrs (so, not to ones that are returned from functions, for example).
b = a;

Anyway, as we see, in your case you won't be able to transfer the ownership to another object: Your object will in effect be non-copyable. And in the next C++ Standard, it will be non-movable if you stay with scoped_ptr.

For implementing your class with scoped_ptr, watch that you either have one of these two points satisfied:

  • Write an destructor (even if it's empty) in the .cpp file of your class, or
  • Make Owned a completely defines class.

Otherwise, when you would create an object of Example, the compiler would implicitly define a destructor for you, which would call scoped_ptr's destructor:

~Example() { ptr.~scoped_ptr<Owned>(); }

That would then make scoped_ptr call boost::checked_delete, which would complain about Owned being incomplete, in case you haven't done any of the above two points. If you have defined your own dtor in the .cpp file, the implicit call to the destructor of scoped_ptr would be made from the .cpp file, in which you could place the definition of your Owned class.

You have that same problem with auto_ptr, but you have one more problem: Providing auto_ptr with an incomplete type is undefined behavior currently (maybe it will be fixed for the next C++ version). So, when you use auto_ptr, you have to make Owned a complete type within your header file.

shared_ptr doesn't have that problem, because it uses a polymorphic deleter, which makes an indirect call to the delete. So the deleting function is not instantiated at the time the destructor is instantiated, but at the time the deleter is created in shared_ptr's constructor.

Johannes Schaub - litb
A: 

I just posted a follow-up question, edited into the original question, please see above.

Ray Hidayat