Re: [boost] Filesystem Review

From: Beman Dawes (bdawes_at_[hidden])
Date: 2002-09-23 06:05:35

At 02:09 PM 9/21/2002, Jeff Garland wrote:

 >First off, I'll say that I think this is a really important library.
 >In addition, over the last few months Beman has done a excellent job
 >of methodically working through the issues, listening to suggestions,
 >and improving the library.  So I am a strong supporter of the filesystem
 >lib being included into Boost and look forward to future enhancements.
 >My review of this library actually started months ago, and has included
 >writing several programs which I have tested out on Linux and under
 >Cygwin with various gcc compilers.

Thanks! Review comments based on actual use are especially appreciated.

 >
 >Some overall comments:
 >
 >* My only 'serious concern' is that we make sure the library the
 >  library can expand to handle different types of 'file systems'
 >  'simultaneously'.  That is, it should be perfectly possible
 >  to provide a path/directory iterator implementation for a
 >  .tar file.  I don't see why this couldn't be done by providing
 >  an implementation of the current interfaces in a tar_fs namespace.
 >  Perhaps there are better solutions?

Well, there are certainly more complex solutions. Create some kind of 
policy based wrapper template, and then turn everything in the current 
library into the default policies. But your decoupled solution is a lot 
better in the real world, IMO, because it takes advantage of the design of 
the Filesystem Library without forcing the two to interact.

 >* Simple/more examples needed.  The examples directory contains the
 >  new status tools, not small concise examples of usage.  This needs
 >  to be fixed.  In that regard I have attached a couple of simple
 >  programs that use fs to implement trivial variants of 'ls', 'mv',
 >  and 'cp'.  Feel free to use them as examples.

Thanks.  Will do.

 >Directory Iterator comments:
 >
 >* I couldn't get the recursive_iterator working.  Without docs, its
 >  not clear what the Predicate stuff is.  This needs to be documented
 >  and should be part of the library.  Unless there is good reason
 >  recursive behavior should just be part of the  directory_iterator
 >  constructor.  That is:
 >
 >  fs::path p(...some path...);
 >  fs::directory_iterator(p, fs::recursive); //default is non-recursive

I hadn't thought of that. I'll try to work out the details to see how it 
would work in practice. It might be a nice symplification.

Predicate was partially just an experiment on my part, because I noticed 
that recursive directory iteration often involved filtering.  I wanted to 
see how a Predicate would work, per discussions on the Boost list last 
Winter.

 >Path Comments
 >
 >* I agree with John and others that the operator<< on path is an odd
 >  interface.  Of the suggestions on the table, my preference is for
 >  'append'. I prefer to avoid overloading mathematical operators for
 >  non-numeric types.  It's just too disorienting for users.  It's
 >  one thing to abuse operators in the context of a meta-programming
 >  library or spirit where the entire library context is outside of
 >  'traditional' C++ programming paradigms.  fs just doesn't fit
 >  that mold.

Well, mold or no mold, operator<< has worked well in practice.  It writes 
well, it reads well, it doesn't seem to be error-prone.  Changing to 
operator / will make it even better.

I understand the concerns.  I had them too, but the "append" interface was 
not good to use in practice.  When I changed to the operator>> interface, 
and changed existing code to use it, the code read ever so much better.

 >* I found the path functions file_path and directory_path odd.  The
 >  fact that they are 'the same' on most systems made the interface
 >  somewhat confusing.  I first assumed that directory_path would
 >  extract the directory portion of the file path, which is of course
 >  the job of branch.  While support of openVMS is a noble nod to the
 >  past, it seems to me that the strange behavior of this particular
 >  filesystem could be handled as a non-standard extension?

The docs changes I made in response John Maddock's comments will improve 
the situation slightly, but while reading your comment it occurred to me 
those two functions could be made free-functions, and moved into a 
sub-namespace (which is part of the official interface) with a name like 
"native" or "detail". Thus they would still be available, but it would be 
ever so much more obvious they aren't intended for everyday use.

(I'm out of time at the moment; will reply later to the rest of your 
comments.)

Thanks,

--Beman