Re: WIP: Implement Filesystem TS

[Jonathan Wakely <jwakely at redhat dot com>]

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.