std::vector of pointers with the rule of three

Question

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;
};

Answer 1

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 cost¹ 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);
        }
}

Answer 2

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.