views:

294

answers:

2

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?

+1  A: 

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>...

Matthieu M.
Hmm... that's annoying as hell :) Thanks!
Billy ONeal
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.
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
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.
+1  A: 

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.

David Rodríguez - dribeas
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
`make_shared` is more efficient in its memory allocations... (less fragmentation, greater speed)
Matthieu M.
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
@David Rodríguez - dribeas: 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