Best way to protect from multiple shared_ptr to the same object

Question

Sending the same pointer into two different shared_ptr's is bad, it causes double deallocation, like thus:

int* p = new int;
std::shared_ptr<int> p1(p);
std::shared_ptr<int> p2(p); // BAD

You can accomplish the same purpose with std::enable_shared_from_this:

class Good: public std::enable_shared_from_this<Good>
{
public:
    std::shared_ptr<Good> getptr() {
        return shared_from_this();
    }
};

int main()
{
    std::shared_ptr<Good> gp1(new Good);
    std::shared_ptr<Good> gp2 = gp1->getptr();
}

But that still doesn't protect against:

class Good: public std::enable_shared_from_this<Good>
{
public:
    std::shared_ptr<Good> getptr() {
        return shared_from_this();
    }
};

int main()
{
    Good* p = new Good;
    std::shared_ptr<Good> gp3(p);
    std::shared_ptr<Good> gp4(p); // BAD
}

which could become a problem if you have code like this:

void Function(std::shared_ptr<Good> p)
{
    std::cout << p.use_count() << '\n';
}

int main()
{
    Good* p = new Good;
    std::shared_ptr<Good> p1(p);
    Function(p);    // BAD
}

Why would I use a regular pointer when there's smart pointers? Because in performance critical code (or for convenience) the overhead of shared_ptr or weak_ptr is undesirable.

To prevent this mistake, I've done:

class CResource : public shared_ptr<Good>
{
public:
    CResource()
    {
    }

    CResource(std::shared_ptr<CBaseControl> c)
        : CResource(c)
    {
    }

private:
    CResource(CBaseControl* p)
    {
    }
};

void Function(CResource p)
{
    std::cout << p.use_count() << '\n';
}

int main()
{
    Good* p = new Good;
    CResource p1(std::shared_ptr<Good>(p));
    Function(p);    // Error
}

This would cause the compiler to error if anyone tries to call Function with a pointer instead of a shared_ptr. It doesn't prevent someone from declaring void Function(std::shared_ptr p) though, but I think that's unlikely.

Is this still bad? Is there a better way of doing this?

Answer 1

The solution is simple: never have a raw pointer own memory in the first place. This pattern:

int* p = new int;
std::shared_ptr<int> p1(p);
std::shared_ptr<int> p2(p); // BAD

Simply shouldn’t exist. Eradicate it from your code base. The only places where new is legitimate in C++11 is as an argument to a constructor call to a smart pointer (or in very low level stuff).

I.e. have code like this:

std::shared_ptr<int> p1(new int);

Or better yet (no naked new involved any longer):

auto p1 = std::make_shared<int>();

Note that it’s fine to use raw pointers in code (but I’d question even that in most C++ code I’ve seen). But if you use raw pointers, don’t let them own the memory. Either point to automatic storage (no resource management necessary) or use a unique_ptr and access the raw pointer via its get member function.