Can I use boost::make_shared with a private constructor?

Question

Consider the following:

class DirectoryIterator;

namespace detail {
    class FileDataProxy;

    class DirectoryIteratorImpl
    {
        friend class DirectoryIterator;
        friend class FileDataProxy;

        WIN32_FIND_DATAW currentData;
        HANDLE hFind;
        std::wstring root;

        DirectoryIteratorImpl();
        explicit DirectoryIteratorImpl(const std::wstring& pathSpec);
        void increment();
        bool equal(const DirectoryIteratorImpl& other) const;
    public:
        ~DirectoryIteratorImpl() {};
    };

    class FileDataProxy //Serves as a proxy to the WIN32_FIND_DATA struture inside the iterator.
    {
        friend class DirectoryIterator;
        boost::shared_ptr<DirectoryIteratorImpl> iteratorSource;
        FileDataProxy(boost::shared_ptr<DirectoryIteratorImpl> parent) : iteratorSource(parent) {};
    public:
        std::wstring GetFolderPath() const {
            return iteratorSource->root;
        }
    };
}

class DirectoryIterator : public boost::iterator_facade<DirectoryIterator, detail::FileDataProxy, std::input_iterator_tag>
{
    friend class boost::iterator_core_access;
    boost::shared_ptr<detail::DirectoryIteratorImpl> impl;
    void increment() {
        impl->increment();
    };
    bool equal(const DirectoryIterator& other) const {
        return impl->equal(*other.impl);
    };
    detail::FileDataProxy dereference() const {
        return detail::FileDataProxy(impl);
    };
public:
    DirectoryIterator() {
        impl = boost::make_shared<detail::DirectoryIteratorImpl>();
    };
};

It seems like DirectoryIterator should be able to call boost::make_shared<DirectoryIteratorImpl>, because it is a friend of DirectoryIteratorImpl. However, this code fails to compile because the constructor for DirectoryIteratorImpl is private.

Since this class is an internal implementation detail that clients of DirectoryIterator should never touch, it would be nice if I could keep the constructor private.

Is this my fundamental misunderstanding around make_shared or do I need to mark some sort of boost piece as friend in order for the call to compile?

Are you sure you need shared_ptr for your impl pointer? boost::scoped_ptr is normally more appropriate and makes things much simpler. Shared_ptr would normally only be used in this case if you wanted DirectoryIterator to be copyable and that the copies should share a single impl instance. In the code you posted, it seems like copies sharing an impl would be an error. Shared_ptr is for when multiple pointers should share ownership of an instance. — Alan, Apr 08 '10 at 16:17

score 5 · Accepted Answer · answered Apr 07 '10 at 07:46

5

You will indeed need to make some boost pieces friend for this. Basically make_shared is calling the constructor and the fact that this is done from within a friend function does not matter for the compiler.

The good news though is that make_shared is calling the constructor, not any other piece. So just making make_shared friend would work... However it means that anyone could then create a shared_ptr<DirectoryIteratorImpl>...

answered Apr 07 '10 at 07:46

Matthieu M.

287,565
48
449
722

1

Hmm... that's annoying as hell :) Thanks! – Billy ONeal Apr 07 '10 at 13:32
The problem of `make_shared` is that it allocates one block of memory and then uses placement `new`, which is why it has to invoke the constructor by itself. I agree it's annoying with regard to your problem. – Matthieu M. Apr 07 '10 at 13:59
1

The problem with doing this is if you migrate to TR1 or C++0x, or even if boost releases an update, you have no guarantee that it will still work. – dvide Apr 07 '10 at 17:19
I've been thinking a bit more about it... and I suddenly realized everything was alright. `DirectoryIteratorImpl` is a private class, it's never exposed in your interface, so a forward declaration in `FileDataProxy` and `DirectoryIterator` is sufficient. Then you can put `DirectoryIteratorImpl` header with your source files (private header) and make its constructor public. Normally nobody should have access to this header, and even if they do they won't be able to do anything with an instance of this class anyway since it never appears in your interface, so you're safe, and problem solved! – Matthieu M. Apr 11 '10 at 11:29
"_The good news though is that `make_shared` is calling the constructor, not any other piece._" How do you know that? – curiousguy Oct 10 '11 at 13:20
@curiousguy: when in doubt, look-up the implementation. – Matthieu M. Oct 10 '11 at 15:14
@MatthieuM. Then you only get the answer about one particular version of this particular library implementation. Any future release might break your assumption. – curiousguy Oct 10 '11 at 15:47
@curiousguy: irrelevant (for Boost). Boost interfaces may change from one version to another, no guarantee at all. I can't give you more guarantee than Boost will, *obviously*. – Matthieu M. Oct 10 '11 at 18:09
@curiousguy: yes, minor versions are for bug fixes and have no interface change, but any major version change (eg, 1.46 to 1.47) may be accompanied by arbitrary interface changes. Of course, in practice, not every library is completely refactored each time (some are not even touched), but they deliberately left the door open to avoid backward compatibility crippling the design/evolution. Boost is the *bleeding* edge of C++ libraries :) – Matthieu M. Oct 12 '11 at 06:18

score 4 · Answer 2 · answered Apr 07 '10 at 08:42

4

Is there a good reason not to use the good old shared_ptr constructor? (If there is one, you might want to take a look at the make_shared implementation and do it)

DirectoryIterator()
   : impl( new detail::DirectoryIteratorImpl() )
{}

This way the call to the constructor is made from the DirectoryIterator class that is already a friend of DirectoryIteratorImpl without opening the door for all other code.

answered Apr 07 '10 at 08:42

David Rodríguez - dribeas

204,818
23
294
489

No, there's nothing wrong with it. But I was told to use `make_shared` on http://stackoverflow.com/questions/2569046/is-there-a-way-to-increase-the-efficiency-of-shared-ptr-by-storing-the-reference/2569211#2569211 . What I've done for now is simply done exactly as you suggest. +1 – Billy ONeal Apr 07 '10 at 13:31
1

`make_shared` is more efficient in its memory allocations... (less fragmentation, greater speed) – Matthieu M. Apr 07 '10 at 13:58
1

I know about the memory fragmentation (you should really meassure), but I read sometime (actually here in SO): *He who sacrifices correctness for performance deserves neither*, which is a nice motto. And in the original linked question, Billy admits that he does not need the performance. If the constructor is private, `make_shared` should not be a friend (that is really close to breaking encapsulation by allowing anyone to construct the object through `make_shared`) – David Rodríguez - dribeas Apr 07 '10 at 15:06
Which is why I've done exactly what you suggest in using shared_ptr's constructor. But you asked why I was using make_shared, and I'm telling you :) – Billy ONeal Apr 07 '10 at 15:11

score 0 · Answer 3 · answered Jul 19 '13 at 04:00

0

You can split your class into interface part and implementation part. The interface part is made public, and the implementation part can have public constructors. However, that means you have to use virtual inheritance.

answered Jul 19 '13 at 04:00

Fuzzier

71
5

Can I use boost::make_shared with a private constructor?

3 Answers3

Linked