Re: [boost] Filesystem Library mini-review

From: Beman Dawes (bdawes_at_[hidden])
Date: 2002-12-16 11:09:08

At 03:05 AM 12/16/2002, Vladimir Prus wrote:

 >Beman Dawes wrote:
 >> A large number of changes to the Filesystem Library have been committed 

 >> to the CVS main trunk.
 >
 >Hi Beman,
 >
 >I've some comments:
 >
 >1. The name 'root_directory' is confusing for me. 'directory' implies
 >    you can stick anywhere dir there, whilie only "/" is allowed.
 >    Unfortunetely, nothing better comes to mind.

I've agonized over the names of many of the functions, and feel that the 
current names are a real improvement over earlier names. Both the 
individual names, and the names as they relate to one another are better.

Nevertheless, if anyone can come up with still better names I'll welcome 
the improvement:-)

 >2. Docs for 'root_directory' say
 >
 >     Returns: If the path contains root-directory, then string(""), else
 >               string().
 >
 >     Portably provides a copy of a path's root-directory, if any. The 
only
 >          possible results are "/" or "". See Path decomposition 
examples.
 >
 >Those paragraphs contradict each other. One says "/" return is not
 >possible,
 >and the other says it is.

The Returns was supposed to say: If the path contains root-directory, then 
string("/"), else string().

Fixed.

 >3.  Docs still use "is_null" in many places.

Fixed, although I only found one place that was incorrect. There was a 
mention of is_null in some naming rationale, but that case is correct.

 >4.  Docs for 'create_directory' don't say what happens if directory_ph is 

 >empty. Consider
 >
 >      path p(.....) ;
 >      create_directory(p.branch_path());
 >
 >I'd personaly prefer if create_directory do nothing when given empty 
path.
 >Now it gives "No such file or directory" error from mkdir.

Interesting; I hadn't considered that.

The alternatives I can think of are to explicit allow it, and "do nothing", 
or to make ph != "" a precondition.

I don't have a strong opinion one way or the other, except that it should 
be specified and not just left unspecified.

Anyone else have an opinion?

(I've also check all other functions taking a path to be sure path("") 
behavior is well specified.)

 >Docs for the same function mention function "branch", which does not 
exist:
 >
 >     if exists(directory_ph)) || !exists(branch(directory_ph))
 >                                         ^^^^^^

Fixed in several places.

 >5.   Docs for 'remove', last paragraph:
 >
 >      "threw and exception" should probably be "threw an exception"

Fixed.

 >6.   Would it be reasonable to introduce a function "create_directories",
 >      similiar in spirit to "remove_all"? That function would create
 >      intermediate directories, not only the leaf one.

Yes, that would be both reasonable and useful. The 
filesystem/convenience.hpp header we've discussed would be a good place.

Care to contribute it?

 >That's all I could spot looking at docs. Maybe, I'll come with more
 >issues after practical use.

Thanks! All good catches!

--Beman