1

I want to enforce shared_ptr for a few classes. I'm using a static factory function to encapsulating private constructors:

#include <memory>

class MyClass
{
    public:

    static std::shared_ptr<MyClass> create() {
        auto a = std::shared_ptr<MyClass>(new MyClass());
        return a;
    }

    private:
        MyClass();
        ~MyClass();
    }
}

This template fails with C2440, (function-style cast) in VS2017, but works fine in VS2015 and I have no idea why. A make_shared-version works fine in both but requires public constructors.

Any idea which option I'm missing?

Student
  • 769
  • 1
  • 6
  • 11
L G
  • 21
  • 3
  • 6
    You destructor has to be public, that's why. – hellow Jul 24 '18 at 12:52
  • Also note, that you have a dangling `}` – hellow Jul 24 '18 at 12:53
  • 5
    Factory functions should generally return `std::unique_ptr`. If the caller doesn't require shared ownership then `std::unique_ptr` is optimal. If the caller does require shared ownership she can simply store the return value in a `std::shared_ptr`, thus upgrading it. The factory function should enable both options and not force shared ownership on the caller. – Jesper Juhl Jul 24 '18 at 12:57
  • Make `std::make_shared` and `std::shared_ptr` friends of `MyClass` – Slava Jul 24 '18 at 12:57
  • @JesperJuhl there is issue with `std::unique_ptr` though – Slava Jul 24 '18 at 12:58
  • 2
    @Slava Care to elaborate? – Jesper Juhl Jul 24 '18 at 12:58
  • 1
    @JesperJuhl `std::make_shared` allocates a continuous block of memory for shared ptr, upgrading `std::unique_ptr` you are loosing that. This is not the case for OP though here. – Slava Jul 24 '18 at 12:59
  • @Slava That's nit-picking and I've never actually seen a case where that *actually* mattered performance wise. – Jesper Juhl Jul 24 '18 at 13:01
  • @JesperJuhl requiring `std::unique_ptr` is nit-picking as well. I never actually seen a case where that mattered performance wise. – Slava Jul 24 '18 at 13:02
  • Having a static shared pointer might be up for debate. They should usually have automatic storage. – Ron Jul 24 '18 at 13:03
  • 1
    @Ron The function is `static`, not the pointer. Also, static objects are handled at the end like automatic objects – NathanOliver Jul 24 '18 at 13:04
  • @NathanOliver Right you are. That's flu side effects from my side. I stand corrected. Still something uneasy about having the shared pointer function for the whole duration of the program imho. – Ron Jul 24 '18 at 13:07
  • 1
    @Slava: (make)`shared_ptr` might delegate construction/destruction to other (helper) classes, so friendship is unfortunately not portable. – Jarod42 Jul 24 '18 at 13:42
  • @Jarod42 in that case it should work as it is as it works in gcc currently IMHO – Slava Jul 24 '18 at 14:02
  • 1
    @Slava - my *point* of returning `unique_ptr` rather than `shared_ptr` was never one of efficiency/performance. It was one of giving the caller control of lifetime and copyability. Things that I find much more important. – Jesper Juhl Jul 25 '18 at 13:08
  • @JesperJuhl I do not see any difference in having shared ptr to a single instance but performance. Control of lifetime and copyability is the same as unique_ptr can be easily upgraded to sharted_ptr – Slava Jul 25 '18 at 13:11
  • @Slava - Exactly, but you don't have that control/option if the factory function insists on returning a `shared_ptr` since you cannot downgrade that to a `unique_ptr` yourself. So, to give the caller the maximum amount of options for how to manage lifetime, the factory function needs to return a `unique_ptr`. And you may *want* a `unique_ptr` to avoid accidentally creating copies (and cycles) which you obviously can't with unique ownership. Shared ownership gives you more rope to hang yourself and it's nice to avoid those pitfalls when you don't *need* shared ownership. – Jesper Juhl Jul 25 '18 at 13:19
  • @JesperJuhl it depends on what factory should do, if it suppose to create single copy to be shared then returning shared_ptr is intentional, if it should create new copy every time, then maybe it should return unique_ptr (and not always, depends on situaition). Insisting, that factory should always return unique_ptr is simply wrong IMHO. – Slava Jul 25 '18 at 13:33
  • @Slava We disagree. No point in arguing this any further. – Jesper Juhl Jul 25 '18 at 13:34
  • @JesperJuhl I get that when the factory shall return a newly created object, the uniqueness of the ownership is part of the contract (and probably a very important part). That's a reason to use `unique_ptr` if you don't expect all users to need shared ownership. But if you know all users will need shared ownership and will upgrade to shared ownership anyway, it makes sense to use `make_shared`. – curiousguy Jul 26 '18 at 03:05
  • "_Insisting, that factory should always return unique_ptr is simply wrong IMHO_" The claim isn't just wrong "in your opinion", it's objectively, provably wrong when qualified with always: any design that relies on being able to call `shared_from_this()`, or any design that relies on `weak_ptr` (and I'm not talking about "design to avoid cycles"), is a lifetime management design that allows sharing, even if initially there is a guarantee that the returned `shared_ptr` has `use_count() == 1`. In that case you need to create a `shared_ptr` not a `unique_ptr`. – curiousguy Jul 26 '18 at 03:08

2 Answers2

2

Looks like VS2017 complains about accessing destructor from std::shared_ptr, so you may want to declare std::shared_ptr as friend of MyClass. For std::make_shared you can use a trick from this answer How do I call ::std::make_shared on a class with only protected or private constructors?

class MyClass
{
public:

    static std::shared_ptr<MyClass> create() {
        struct make_shared_enabler : MyClass {};
        return std::make_shared<make_shared_enabler>();
    }

    // compiles fine for gcc without friend though
    //friend class std::shared_ptr<MyClass>;
private:
   MyClass() {}
   ~MyClass() {}
};

live example

Slava
  • 40,641
  • 1
  • 38
  • 81
  • Thanks, got it. I was actually using that template without remembering the no-private-destructor part. – L G Jul 25 '18 at 13:30
1

In addition to other answers: if you don't want to declare std::shared_ptr as a friend of your class and you don't want to make your destructor public, you can create a shared_ptr with the custom deleter. For that you will need some method from your MyClass that can access the private destructor and call delete. For example:

class MyClass
{
public:
    static std::shared_ptr<MyClass> create() {
        auto a = std::shared_ptr<MyClass>(new MyClass(),
            [](MyClass* ptr) { destroy(ptr); });
        return a;
    }
    static void destroy(MyClass* ptr) { delete ptr; }
    private:
        MyClass(){}
        ~MyClass(){}
};

// later in the source code
    auto ptr = MyClass::create();

You can also declare destroy method as non-static and commit a suicide inside (one of a few situations when it actually makes sense).

pptaszni
  • 3,794
  • 5
  • 19
  • 37