Why make_shared<Object>() gives error with Object with private constructor where as new operator works fine.

[kapil2905]

Hi ppl :),
make_shared is more efficient than shared_ptr(new Object), but why when we use make_shared for instance creation of singleton class, it giver private constructor error, whereas shared_ptr(new Object) works fine but is inefficient.

Below is the code and error

error :

animal_factory.cpp:22:61:   required from here
/usr/include/c++/6/ext/new_allocator.h:120:4: error: ‘AnimalFactory::AnimalFactory()’ is private within this context
  { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
animal_factory.cpp:3:1: note: declared private here
 AnimalFactory::AnimalFactory()

animal_factory.h

#ifndef ANIMAL_FACTORY_H_
#define ANIMAL_FACTORY_H_

#include "animal_prototype.h"
#include "cat_prototype.h"
#include "snail_prototype.h"
#include <map>
#include <string>
#include <memory>
#include <mutex>

class AnimalFactory
{
    private:
        AnimalFactory();
        AnimalFactory(const AnimalFactory &);
        AnimalFactory& operator=(const AnimalFactory&);
        static std::shared_ptr<AnimalFactory> animalFactory;
        typedef std::map<std::string, std::unique_ptr<AnimalPrototype> > AnimalFactoryMap;
        AnimalFactoryMap myMap;
        static std::recursive_mutex mtx;

    public:
        virtual ~AnimalFactory();
        static std::shared_ptr<AnimalFactory> getAnimalFactory();
        std::unique_ptr<AnimalPrototype> getAnimal(const std::string &);
};

#endif 

animal_factory.cpp

#include "animal_factory.h"

AnimalFactory::AnimalFactory()
{
    myMap["cat"] = std::make_unique<CatPrototype>();
    myMap["snail"] = std::make_unique<SnailPrototype>();
}
AnimalFactory::AnimalFactory(const AnimalFactory &) {}
AnimalFactory::~AnimalFactory() {}
AnimalFactory& AnimalFactory::operator=(const AnimalFactory &other) { return *this;}

std::shared_ptr<AnimalFactory> AnimalFactory::animalFactory;
std::recursive_mutex AnimalFactory::mtx;

std::shared_ptr<AnimalFactory> AnimalFactory::getAnimalFactory()
{
    if(animalFactory == nullptr)
    {
        mtx.lock();

        if(animalFactory == nullptr)
            animalFactory = std::make_shared<AnimalFactory>();
            //animalFactory = std::shared_ptr<AnimalFactory>(new AnimalFactory); works fine

        mtx.unlock();
    }
    return animalFactory;
}

std::unique_ptr<AnimalPrototype> AnimalFactory::getAnimal(const std::string &animalType)
{
    return myMap[animalType]->clone();
}

Thanks :)

[gunnerfunner]

Same query under under thread heading that you'd green-ticked the reply: http://www.cplusplus.com/forum/general/211005/
did you try the suggestions from previous and, if so, what results did you get?

[kapil2905]

Dear @gunnerfunner, the answer their gives a workaround to make the above code work. But does not clarify why does this error come. I am looking for the reason of the error and not the workaround.
basically


1) Why does new operator work where as make_shared<Object>() does not work if the constructor is private.

2) Why are we getting this error though we are calling make_shared inside a member function.


Thanks :)

[Cubbi]

make_shared is not a member function of your class, so it has no access rights to your class's private members.

(technically you could grant those rights by friending it, but if you saw the error message, it wasn't even make_shared itself that attempted to access the constructor, but another function called from it, and you can't just arbitrarily friend internal functions from the standard library)

[mbozzi]

Specifically, the requirements on make_shared and allocate_shared are here;
http://eel.is/c++draft/util.smartptr.shared.create#1
the first paragraph's requirement is not met, because AnimalFactory() is private.

As @cubbi says, private member functions are only accessible from inside the class, not from standard library code.

You can write a thunk for make_shared() by leveraging a derived class, which probably shouldn't be part of the interface... if you're willing to make the constructor protected:

# include <memory>
class A { protected: A() = default; A(int, int) {}; };

namespace {
  struct A_derived final: A {
    template<typename...Args>
    A_derived(Args&&... args): A(std::forward<Args>(args)...) {}
  };
}

template<typename... Args>
std::shared_ptr<A> make_shared_A(Args&&... args)
{ return std::make_shared<A_derived>(std::forward<Args>(args)...); }

int main() {
  auto some_a1 = make_shared_A();
  auto some_a2 = make_shared_A(1, 2);
}

http://coliru.stacked-crooked.com/a/74bf2d9f2dc248a9
Maybe someone can come up with a way to do this more nicely.

Note: (usually) the primary benefit of make_shared is exception-safety, not efficiency. Since it's a function, it forces the constructor arguments to be evaluated before the memory is allocated, which is not required in the case of plain new-expressions. Consider: std::shared_ptr<T>(new T(might_throw());

Also, handing out shared_ptr is questionable.

Multiple invocations would yield independent shared_ptrs, leading to undefined behavior upon destruction. While you've made it impossible to do by caching the result of the first call, that seems opaque and could do with a comment.

Further, unique_ptr should be preferred unless you really have a need for shared ownership -- there is a danger of caching shared pointers cross-module and causing leaks that way.

[kapil2905]

Thanks alot @mbozzi for this detailed reply :)