Management of a vector of pointers as a class member

Question

I am in a rather specific situation that requires me to use a vector of raw pointers as a class member:

I need to keep a list of abstract objects (I understood I could not use vector<AbstractClass> directly).
I can't use Boost or std::tr1, so no smart pointer library (because of constraints from the hardware I'm using).

Here is a basic example of what I need:

#include <vector>

class Foo
{
    virtual void foo() {}
};

class Bar : public Foo
{
    void foo(){}
};

class FooBar
{
    vector<Foo*> list;
    void add(){ list.push_back(new Bar()); }
};

Coming from Java, I am very scared of pointers and memory leaks. This post initially suggests to delete the objects manually when the object goes out of scope. Is it enough to do this in the destructor of FooBar?

class FooBar
{
    vector<Foo*> list;
    void add(){ list.push_back(new Bar()); }

    ~FooBar(){
         for(vector<Foo*>::iterator it=list.begin(); it!=list.end(); it++)
             delete *it;
    }

};

By the rule of three, I guess I also have to implement a copy constructor and an assignment operator. Is the following (based on this and that posts) a correct implementation ?

FooBar::FooBar(const FooBar& orig) : list(orig.list.size()) {
    try {
        vector<Foo*>::iterator thisit = list.begin();
        vector<Foo*>::const_iterator thatit = orig.list.cbegin();

        for (; thatit != orig.list.cend(); ++thisit, ++thatit)
            *thisit = *thatit;  // I'm okay with a shallow copy

    } catch (...) {
        for (vector<Foo*>::iterator i = list.begin(); i != list.end(); ++i)
            if (!*i)
                break;
            else
                delete *i;

        throw;
    }
}

FooBar& operator=(const FooBar& orig){
    FooBar tmp(orig);
    swap(tmp);
    return *this;
}

`// I'm okay with a shallow copy` This together with the destructor implementation will break your neck. You'll try to free the allocations made multiple times. — πάντα ῥεῖ, Mar 11 '16 at 10:09
Can't understand "no smart pointer library". You can get the raw pointer from smart pointer anytime. — songyuanyao, Mar 11 '16 at 10:10
@πάνταῥεῖ, so is the destructor wrong or the copy constructor wrong ? — Eric Leibenguth, Mar 11 '16 at 10:14
if `no smart pointer library (because of constraints from the hardware I'm using).` then write your own simple shared smart-pointer with counting of links. It is not hard as minded. Your current code (copy method) will leaks memory — nikniknik2016, Mar 11 '16 at 10:14
@EricLeibenguth Your copy constructor is wrong. Also what guarantees that the vectors in use have the same number of elements? — πάντα ῥεῖ, Mar 11 '16 at 10:15
@MohamadElghawi, the try-catch is probably unnecessary indeed. I copied it from the post mentioned above, but there a deep copy was performed. — Eric Leibenguth, Mar 11 '16 at 10:15
@EricLeibenguth If the ownership of the `Foo`s should be shared between the `FooBar`s, the destructor is wrong (and you need to add lifetime management). If each `FooBar` should contain unique `Foo`s, the copy constructor is wrong. — molbdnilo, Mar 11 '16 at 10:16
@nikniknik2016, any example of implementation you would direct me to? — Eric Leibenguth, Mar 11 '16 at 10:16
@FatihBAKIR, I did not yet. I copied it from a referenced post and assumed it would not pose a big challenge to implement... — Eric Leibenguth, Mar 11 '16 at 10:21
@EricLeibenguth, for instance, this is steb-by-step implementation guide: http://www.codeproject.com/Articles/15351/Implementing-a-simple-smart-pointer-in-c — nikniknik2016, Mar 11 '16 at 10:21
Thanks for all your comments. I guess I'll go around the problem with a homemade smart pointer implementation. Though I'm still curious to see what a clean solution would look like with raw pointers. — Eric Leibenguth, Mar 11 '16 at 10:26

score 1 · Accepted Answer · answered Mar 11 '16 at 10:30

Your code has some major flaws as far as I can tell. First of all, your first code is correct, and yes, your destructor does what it should do, as long as your class owns the objects and doesn't get copied.

Also, your copy constructor and assignment operator looks problematic. First of all, if you are okay with shallow copying, you don't even have to write the functions anyway, std::vector's copy constructor and assignment operators do what you have done manually anyway. I don't think you need that try-catch block. By the way, where is the implementation for that swap in your assignment operator?

For your purposes, a very small reference counting scheme would work:

class FooPtr
{
     Foo* _raw;
     size_t* _ctl;

     FooPtr(Foo* ptr) : 
         _raw(ptr), _ctl(new size_t(0))
     {
     }

     template <class T>
     FooPtr(T* ptr) : FooPtr(static_cast<Foo*>(ptr)) {}

     FooPtr(const FooPtr& rhs) : 
         _raw(rhs._raw), _ctl(rhs._ctl)
     {
         ++(*_ctl);
     }

     ~FooPtr()
     {
        if (_raw == nullptr) return;
        --(*_ctl);
        if (*_ctl == 0)
        {
            delete _raw;
            delete _ctl;
        }
     }

     FooPtr& operator=(FooPtr ptr)
     {
         std::swap(*this, ptr);
         return *this;
     }

     Foo* operator->()
     {
         return _raw;
     }

     Foo& operator*()
     {
         return *_raw;
     }
}

Your class now looks like this, no destructor, copy ctor or assignment operator needed:

class FooBar
{
     vector<FooPtr> list;
     void add(){ list.emplace_back(new Bar()); }
}

I've written this quickly, it might contain bugs though it gives you the basic idea I suppose. Also, I've used some c++11 features like emplace_back and nullptr but they can easily be converted in non 11 code.

Thanks! I'm going for this solution (although I started with the implementation from @nikniknik2016 's link). — Eric Leibenguth, Mar 11 '16 at 10:40

Simple · Answer 2 · 2016-03-11T10:48:31.233

I would write a small wrapper around a vector that frees the elements for you:

template<class Container>
void delete_elements(Container& cont) {
    typedef typename Container::reverse_iterator iterator;
    iterator end = container.rend();
    for (iterator i = container.rbegin(); end != i; ++i) {
        delete *i;
    }
}

struct deep_copy {
    template<typename T>
    T* operator()(T const* const other) {
        return other->clone();
    }
}

template<typename T>
struct ptr_vector {
    std::vector<T*> container;

    ptr_vector() { }

    ptr_vector(ptr_vector const& other) {
        std::vector<T*> tmp;
        tmp.reserve(other.container.size());

        try {
            std::transform(other.container.begin(), other.container.end(),
                    std::back_inserter(tmp), deep_copy());
        }
        catch (...) {
            (delete_elements)(tmp);
            throw;
        }

        container.swap(tmp);
    }

    ptr_vector& operator=(ptr_vector other) {
        container.swap(other.container);
        return *this;
    }

    ~ptr_vector() {
        (delete_elements)(container);
    }
};

Now you can just use a ptr_vector<Foo> v and use v.container to add elements. Note: you should add a clone member function to your base class so you don't slice when you want to copy.

Thanks. I haven't tested it, but it looks like a good alternative. — Eric Leibenguth, Mar 11 '16 at 10:48

Management of a vector of pointers as a class member

2 Answers2