Efficient way to add shared_ptr parameter to a container?

Question

I have a container class(i.e. Container) that holds shared_ptr initialized by different subclass of class Base. I have implemented the idea as the following code.

Question 1> Overall, can you find any potential problems in the following code? I am interested in the best practice and also the currently code passed vs2010 and output the expected results already.

Question 2> Is it the right way that I design the Container::Add signature so that the client needs to pass in a shared_ptr?

#include <iostream>
#include <vector>
#include <memory>

using namespace std;

class Base
{
public:
    virtual void PrintMe(void) = 0;
    virtual ~Base() = 0 {} // **Updated based on comments from [bdonlan]**
};

class SubClassA : public Base
{
public:
    virtual void PrintMe(void)
    { cout << "This is SubClassA" << endl; }
};

class SubClassB : public Base
{
public:
    virtual void PrintMe(void)
    { cout << "This is SubClassB" << endl; }
};

typedef std::shared_ptr<Base> BasePtr;
typedef std::vector<BasePtr>::iterator VecIter;

class Container
{
public:
    void Add(BasePtr ptr)
    { vec.push_back(ptr); }

    void PrintAll()
    {
        for ( VecIter iter = vec.begin() ; iter < vec.end(); ++iter )
        {  (*iter)->PrintMe(); }    
    }

private:
    vector<BasePtr> vec;
};    

int _tmain(int argc, _TCHAR* argv[])
{
    Container con;    
    BasePtr ptrA(new SubClassA);
    BasePtr ptrB(new SubClassB);    
    con.Add(ptrA);
    con.Add(ptrB);    
    con.PrintAll();    
    return 0;
}

You can find it here: http://codereview.stackexchange.com/questions :) — Thomas Russell, Aug 28 '11 at 19:55
In C++11 you can write it like that: `for (auto it: vec) it->PrintMe();` — dimitri, Aug 28 '11 at 20:46

score 1 · Answer 1 · answered Aug 28 '11 at 16:18

1

You need to add a virtual destructor to Base; the BasePtr shared pointer will try to delete the subclasses as a Base *, but this is only legal if Base has a virtual ~Base. It should be enough just to add a public virtual ~Base() { } to the Base class body.

Apart from that I don't really see any major problems.

answered Aug 28 '11 at 16:18

bdonlan

224,562
31
268
324

Good suggestion and I have updated the code based on your suggestions. – q0987 Aug 28 '11 at 16:20
@q0987, not a pure virtual destructor - just an ordinary virtual destructor. So no `= 0` for the destructor. – bdonlan Aug 28 '11 at 16:22
@ bdonlan, why not? a pure virtual destructor is good for an abstract class. Although here, there already contains a pure virtual function `PrintMe`. Based on my understanding, there is no big difference here between 'pure virtual destructor' or 'virtual destructor' – q0987 Aug 28 '11 at 16:30
@q0987, ah, sorry, my mistake. In this case it's the same to have a pure virtual or ordinary virtual destructor, see http://stackoverflow.com/questions/1219607/why-do-we-need-a-pure-virtual-destructor-in-c – bdonlan Aug 28 '11 at 16:40

Efficient way to add shared_ptr parameter to a container?

1 Answers1