1
\$\begingroup\$

I have a class that has a std::vector of pointers, I'm NOT going to give any of those pointers to objects outside of it, I mean, I'm not going to share the pointers. - I was reading that isn't everywhere that we will use smart pointers and that raw pointer isn't useless as long that someone is responsible for deleting it. The problem with that class is that it can be inherited and if I don't use the rule of three all the classes that inherits from will have a copy of the vector with the pointers then the destructor of those objects will result in double delete. - Is it safe or I still need to use std::shared_ptr in this case?

class Keeper
{
public:
    Keeper() {}
    Keeper(const Keeper&) = delete;
    Keeper& operator=(const Keeper&) = delete;

    virtual ~Keeper()
    {
        for (int i = 0; i < pointers.size(); ++i)
        {
            delete pointers[i];
        }
    }

    void push_back(Pointer * pointer)
    {
        pointers.push_back(pointer);
    }

    // erase method...
protected:
    std::vector<Pointer *> pointers;
};
\$\endgroup\$
  • \$\begingroup\$ Possibly off topic for asking about basic information instead of a code review. The answer is that the destructor of the inheriting class does not delete the pointers, so there are no double deletes. Since you do not share the pointer shared_ptr is obviously wrong. You could use unique_ptr, but vector<Pointer> would be much better. Btw. what is the purpose of the private pointers? \$\endgroup\$ – nwp Nov 11 '14 at 19:05
  • \$\begingroup\$ Actually it's protected, I will edit. - The class that inherits can use the pointers of the vector. - vector<Pointer> or vector<Pointer *>? \$\endgroup\$ – yayuj Nov 11 '14 at 19:12
  • \$\begingroup\$ This seems incomplete. First, is this actually supposed to be a templated class or is Pointer a concrete type? Second, there appears to be no way to access the pointers once they're in the Keeper. The only way I can see that could be useful is if the sole point to this class is to do something like garbage collection. \$\endgroup\$ – Edward Nov 11 '14 at 19:25
  • \$\begingroup\$ Pointer is just a type (interface/abstract class) and Keeper accepts classes that inherits from Pointer. - Now I'm wondering... if my vector is private, does the inheriting class copy the vector? And isn't if safe that the vector is protected and the inheriting classes can access it as friend? If the inheriting class delete one of those pointers of the vector will result in a mess, right? \$\endgroup\$ – yayuj Nov 11 '14 at 19:32
  • \$\begingroup\$ Inheriting classes do not copy anything. The base class has the vector and the inheriting class does not. It is the base class's responsibility to clean up its members and the inheriting classes are not supposed to delete other people's data. The code is correct as is as far as deleting of the pointers is concerned (it is just unnecessary), there is no problem with inheritance here. \$\endgroup\$ – nwp Nov 11 '14 at 19:57
4
\$\begingroup\$

Is it safe? To a point.

Naming

I hope the name Pointer is just for illustration purposes as otherwise that is a pretty terrible name.

The changes I would make are:

Accepting pointers must indicate ownership.

Your current interface does not indicate that it is taking ownership of the pointers. So as a user of your class I need to dig inside and find out if you are taking ownership or not before I can use it.

void push_back(Pointer * pointer);

I would change the interface to specifically take a unique_ptr that way people know that you are taking ownership of the object.

void push_back(std::unqiue_ptr<Pointer> pointer);

No confusion here.

Storage of pointers.

I have no problem with you storing a vector of pointers or a vector of smart pointer. Either makes sense. As long as you define the Copy Constructor/ Assignment Operator and Destrtuctor if you use pointer (like you have). You could argue that keeping an array of smart pointers will save you some work in coding (tiny (probably insignificant cost1 of managing the data). Alternatively you can use a container specifically designed to hold pointers.

 std::vector<Pointer*>                 data;  // Add Rule of Three.
 std::vector<std::unique_ptr<Pointer>> data;  //
 boost::ptr_vector<Pointer>            data;  // Takes ownership of pointers.
                                              // Only exposes members as references
                                              // to actual object (not pointer) and thus
                                              // makes using it with standard algorithms
                                              // much easier and intuitive.

To Clarify because of comments: I suspect for most compilers the cost of std::unique_ptr will be zero at runtime. But it is something worth validating with your compiler before assuming.

Access from derived types.

Personally I would not give direct access to the vector from derived class like that.

protected:
      std::vector<Pointer *> pointers;

Even if you have a vector of unique_ptr it is still to easy for the derived class to break encapsulation and do something you don't intend. You should provide them with a safer interface. How you do that depends.

If you know none of the members will be NULL then I would provide them with a function that returns a reference.

 protected:
       // Anybody that wants to access the data gets a reference.
       Pointer&    data(std::size_t index) {return *pointers[index];}

       // Note: the above code just uses the original code as a base line.
       //       If you change the storage medium I would probably still
       //       provide this as an interface layer so that I don't
       //       break encapsulation.
       //
       //       Of course there are a lot of caviats that depend on how
       //       this will be used. But without further context this is
       //       the best I can do at the moment.


 private:
       std::vector<Pointer*> pointers;

If you want derived classes to modify the class you may need to think of something else. But you have not provided enough context for me to go further.

Iterator

Based on comments just showing how easy it would be to throw an iterator together that can be used without exposing implementations details. Note: this one is not complete but shows enough of how it would work (and I was slightly bored at the time and wanted to write some code).

class MyIterator
{
    Keeper* parent;
    int     index
    public:
        MyIterator(Keeper& parent, int index)
           : parent(&parent)
           , index(index)
        {}
        MyIterator& operator++()
        {
            ++index;
            return *this;
        }
        MyIterator operator++(int)
        {
            MyIterator result(*this);
            ++(*this);
            return result;
        }
        Pointer& operator*()
        {
            return parent->data(index);
        }
}
\$\endgroup\$
  • \$\begingroup\$ Don't worry Pointer is just an illustration, I'm kind of addicted to design then my codes (at least I try) to follow principles and patterns.- You're right about using std::unique_ptr to indicate ownership and the implementation of a method to return just a reference, but what if the inheriting classes need to interact over all the pointers in the vector? (even as reference)? \$\endgroup\$ – yayuj Nov 11 '14 at 21:51
  • \$\begingroup\$ Should not be that hard to create iterators. But you can also just provide the size through the interface then they can access it iteratively by incrementing the counter from zero to size. \$\endgroup\$ – Martin York Nov 11 '14 at 22:00
  • \$\begingroup\$ - Reading your code I realized that your idea of returning reference in order to provide a safer interface you are using raw pointer instead of smart pointers, knowing that, are you assuming that it's better to use raw pointers in that case or switch to smart pointers and use get method in order to retrieve the point from std::unique_ptr? \$\endgroup\$ – yayuj Nov 12 '14 at 18:05
  • \$\begingroup\$ No. I was just using the original code. I have no preference over array of pointer or array of smart pointers. But either way I would probably provide a tighter interface returning a reference with the data() method (assuming this would work (it can't work if you allow NULL values for example)). \$\endgroup\$ – Martin York Nov 12 '14 at 19:26
  • 1
    \$\begingroup\$ @Deduplicator: Updated wording. \$\endgroup\$ – Martin York Nov 13 '14 at 13:09
1
\$\begingroup\$

The rule of three applies only to constructing new instances of a class (mostly from copying an existing instance of the same class) via the constructor, copy constructor and copy assignment. It has nothing to do with class inheritance.

When you inherit from a class and instantiate the derived class you only get one instance (not an instance of the base and an instance of the derived) and therefore only one of the std::vector. The base classes virtual destructor will cleanup the vector correctly regardless of the inheritance tree and as you have made the class non-copyable nothing else can take ownership of the vector meaning it will only be cleaned up once.

That being said this is a code review site and there is a design issue with using raw pointers in this way:

The issue with raw pointers is that they don't convey ownership. The push_back method takes a raw pointer but the calling class has no explicit way of knowing that they are transferring ownership of the pointer into the keeper class and therefore may end up deleting the pointer themselves. This would cause the pointer to be deleted twice.

If the keeper class is the sole owner of the pointer (and it probably is as it deletes it and you explicitly state that the pointers are not shared) then it should take a unique_ptr.

class Keeper
{
public:
    Keeper() = default;
    Keeper(const Keeper&) = delete;
    Keeper& operator=(const Keeper&) = delete;

    virtual ~Keeper(){}

    void push_back(std::unique_ptr<Pointer> pointer)
    {
        pointers.push_back(std::move(pointer));
    }

    // erase method...
protected:
    std::vector<std::unique_ptr<Pointer>> pointers;
};

This means that calling push_back explicitly transfers ownership to the keeper class and this is enforced by the compiler. It also has the added benefit of cleaning up after itself when the class goes out of scope without the need to manually delete the memory held by vector.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.