WIP: Implement Filesystem TS

[Jonathan Wakely]

On 05/08/14 15:18 +0200, Marc Glisse wrote:
>On Mon, 4 Aug 2014, Jonathan Wakely wrote:
>
>>>(N4099 writes --end() in a few places, I don't remember seeing 
>>>text explaining that this is ok even if end() returns a pointer, 
>>>while we do have text explaining what end()-1 means)
>>
>>The synopsis in [class.path] declares path::iterator as a class type,
>>so it can't be a pointer.
>
>So we can't directly use list::iterator for instance because it is a 
>typedef and not a new class? And even less vector::iterator which on 
>some implementations is a pointer.

The TS says it's a class, which is detectable by users (as they can
refer to it using the elaborated-type-specifier "class path::iterator"
which is not valid for a typedef name)

>If we want to force iterators to be classes, why not, but sneakily 
>starting with path seems strange. typedef unspecified would be more 
>consistent with the current library.

I don't see why path::iterator being a class implies anything about
any other iterators, only about path::iterator

>Or are there functions overloaded on path::iterator that I missed?

No, I don't think so. I think it's probably overspecification.

>In my opinion, the path iterator should be an input iterator that 
>additionally supports operator-- etc (all properties of a 
>bidirectional iterator except for return-a-reference). The concepts 
>should adapt to what we want to do, we shouldn't try to shoehorn our 
>designs into bogus concepts.

It's a bit late to be doing design review on the TS now, I'm just
implementing the spec because it's about to be published by ISO :-)

I suggest you open issues against the TS, so that we can fix things
before anything becomes part of C++17.

>>My recursive_directory_iterator implementation instead uses
>>shared_ptr<stack<pair<_Dir, directory_iterator>>>, where the
>>shared_ptr<_Dir> held by each directory_iterator shares ownership with
>>the shared_ptr<stack<...>> (using the shared_ptr aliasing constructor)
>>so there is only a single control block for the whole group of
>>objects.
>
>Ah, ok, I really should pay attention to new classes, I had made some 
>assumptions on how shared_ptr works that don't match reality and thus 
>I didn't realize this was possible.

:-)

>It looks like, as is often the case, the iterator interface is a pain 
>to implement, where a range interface would be a bit easier (the stack 
>doesn't need to be in a shared_ptr), and a foreach interface (or 
>output iterator) would be trivial (especially if we don't mind 
>recursive calls crashing for huge directory depths). It is a good 
>thing there will be slow system calls in the middle or all this 
>overhead might be a bit painful.
>
>The range interface looks strange. Not sure why they are reusing 
>directory_iterator instead of creating a separate directory_range. It 
>doesn't matter much though.

The Boost.Filesystem design has been around for years, before ranges
became so fashionable, so the reasons might be historical.

>>Does that make it clearer?
>
>Yes, thanks a lot for the explanations.
>
>On Tue, 5 Aug 2014, Jonathan Wakely wrote:
>
>>On 04/08/14 23:36 +0100, Jonathan Wakely wrote:
>>>One solution would be for recursive_directory_iterator to not use
>>>directory_iterator, but work with a stack of unique_ptr<_Dir> objects
>>>instead, re-implementing the required directory_iterator members that
>>>operate on the _Dir object. I didn't try that, but I think it would
>>>work.
>>
>>Or it could just use std::stack<_Dir> ... but I think I did try that
>>and hit some issues. Maybe I should try again because ...
>
>stack<_Dir> makes sense to me...

I don't remember why it didn't work, but it might have been with an
earlier version of the code where _Dir was a very thin wrapper over
DIR* and so didn't provide enough functionality for stack<_Dir> to
work. I moved more functionality into the _Dir class and so it might
work now.

>>I've realised there's a problem with this code, I'm not correctly
>>breaking the reference cycles when a recursive_directory_iterator is
>>destroyed with depth() != 0, so leaking memory. I'll fix that ...
>
>Sounds more like a detail than a true design issue.

Yes, I just need to make sure I break the cycles before dropping a
reference count on the shared_ptr, but the need to do that manually is
a bit annoying and much less convenient than just relying on
shared_ptr.

Thanks very much for all the useful comments.