Factory method creating shared_ptr object

Question

When using a factory to create an object, such as in the example below, in certain circumstances the objected wrapped by the shared_ptr apparently gets deleted during the return process (during debugging the object is created OK but when it is assigned to this->xs an exception is thrown). When I change the factory method to return a raw pointer, and the Link::xs member to be a unique_ptr the code runs fine. What is going on under the hood with shared_ptr to cause it to behave this way? Is it related to the fact that shared_ptr<CrossSection> is wrapping a Circular object? Testing has been done using MS Visual C++ 2012.

class Link
{
private:
    std::shared_ptr<xs::CrossSection> xs;
public:
    void parseXsection(const std::vector<std::string>& parts);
    std::shared_ptr<xs::CrossSection> getXs() { return this->xs; }
};
void Link::parseXsection(const std::vector<std::string>& parts)
{
    this->xs = xs::Factory::create(parts[1]);
}

namespace xs
{
    class CrossSection
    {
    };
    class Circular : public CrossSection
    {
    };
    class Dummy : public CrossSection
    {
    };
    class Factory
    {
    public:
        static std::shared_ptr<CrossSection> create(const std::string& type);
    };
    std::shared_ptr<CrossSection> Factory::create(const std::string& type)
    {
        if (geom == "circular")
        {
            return std::shared_ptr<CrossSection>(new Circular());
        }
        else
        {
            return std::shared_ptr<CrossSection>(new Dummy());
        }
    }
}

Your code has undefined behaviour because `Factory::create` may not return anything when it has to return a `shared_ptr`. You should fix that first. — juanchopanza, Aug 01 '14 at 16:43
I added some more code to fix that problem but in my real implementation that is taken care of. — LucasMcGraw, Aug 01 '14 at 16:47
I'm sure it was just for the sake of simplicity, but in general it's probably a better idea to use enumerated types and a `switch` statement to do something like that. It is more efficient than doing string comparison, and you have a single place in which you can modify all of your comparison options. — Mitchell Tracy, Oct 20 '17 at 14:04

score 7 · Accepted Answer · answered Aug 01 '14 at 16:51

7

So, Martin has one option for fixing a destructor problem. You could add a virtual destructor.

However, because you're using std::shared_ptr, which employs a bit of type erasure, you could do a smaller fix:

std::shared_ptr<CrossSection> Factory::create(const std::string& type)
{
    if (geom == "circular")
        return std::shared_ptr<Circular>(new Circular());
    else
        return std::shared_ptr<Dummy>(new Dummy());
}

Or, even better:

std::shared_ptr<CrossSection> Factory::create(const std::string& type)
{
    if (geom == "circular")
        return std::make_shared<Circular>();
    else
        return std::make_shared<Dummy>();
}

answered Aug 01 '14 at 16:51

Bill Lynch

80,138
16
128
173

How would this help any more than having a virtual destructor in `CrossSection`? – R Sahu Aug 01 '14 at 17:10
@RSahu: It helps the same amount. You are aware that the `shared_ptr` when constructed for the first time will retain the knowledge of the original pointer's type, and call that destructor. So here, as long as the original shared_ptr constructor has the correct type (which is what my change is all about), then the correct destructor will be called – Bill Lynch Aug 01 '14 at 17:17
No, I didn't know that `shared_ptr` retains the knowledge of the original pointer's type. +1. – R Sahu Aug 01 '14 at 17:35
@RSahu [__This post__](http://stackoverflow.com/questions/5913396/why-do-stdshared-ptrvoid-work) has a resonable explanation on how it works. – Bill Lynch Aug 01 '14 at 17:59

score 4 · Answer 2 · edited May 23 '17 at 11:33

You definitely need to define a virtual destructor in your CrossSection base class in order to use polymorphism, i.e. in order to declare derived classes and use them in place of the parent class (so generally almost everytime you want to use derived classes ...)

class CrossSection {
  public:
    virtual ~CrossSection() { /* Nothing to do here ... */ }
};

See e.g. When to use virtual destructors? or Should every class have a virtual destructor? for more explanations.

PS: I cannot say right now, if this is the cause for your problems with shared_ptr, but it looks a lot like the kind of problems you might encounter if you forget virtual destructors ...

Factory method creating shared_ptr object

2 Answers2