RE: [boost] Re: the first version of abstract path manipulation c lass (interface)

From: Jason Stewart (res0054p_at_[hidden])
Date: 2002-01-25 12:54:51

>According to customary, almost documented Boost coding guidelines, some
>changes to your code are in order:

This class (along with the Filename class) were developed for internal use. 
I submitted it as a possible implementation for the pathname handling. The 
reason for the inconsistencies is that I added the policy stuff at the last 
minute just to illustrate a problem. I would plan to make these changes if 
we wanted to adopt these classes.

>Shouldn't equal() and less() in Lowercase and Uppercase coerce the case of
>the two strings first?  (I think the best way to handle that is for both to
>force the strings to lower case for comparison, as there are non-English
>language problems going from lower to upper case.)

I don't know, maybe they should, I guess for broad acceptance it might 
should use locals (I've never used locals).

>ForwardSeparator::get() strikes me as a bad name, but I can't think of a
>better one offhand.

Same here, I didn't like it but I couldn't think of anything else.

>ForwardSeparator::isSeparator()'s (is_separator()'s) implementation should 
>just be...

Possibly, in my software, I tend to convert all separators to forward 
slashes since it works on all platforms that I deal with. However, users 
still tend to enter backslashes on windows. Since I want to convert them I 
like isSeparator to return true for both.

>isRedundant(), in your "Sep" classes, won't work for any filesystem (of
>which we haven't established the existence, of course) that uses multiple
>characters for the separator.  But, more than that, I don't see the value of
>the two-parameter function.  Why not just a "duplicates_allowed()" function
>that returns a bool and controls Pathname's behavior?  If Pathname is
>written in terms of is_separator(), then you get the detection of "\\/" in a
>string where both "\\" and "/" are valid separators.

If someone points out a filesystem with multiple characters for separators 
we may have to change it. However, I hate to make big allowances for this 
if it is not needed. Using a "duplicates_allowed" might be a good idea. 
When I wrote this part yesterday I needed a simple helper function and this 
worked well but I can change it.

>Pathname's "Sep" template parameter should be "Separator," if not
>"SeparatorPolicy."  (Similarly, "Case" may be better as "CasePolicy," if
>only to clarify its purpose.)

I can see this, I know the STL implementations tend to use very short names 
but I don't really care.

>Pathname's converting ctors should be explicit.

Agreed.

>Pathname::Get() should be named "to_string" or something like that.
>"normalized" should not be a bool; instead, create an enum with two
>enumerators: normalized and no_tail.  (I couldn't think of a good pair of
>names, but we can work on that.)  The point is that in calling code, the
>Boolean value is not meaningful:
>
>     pathname pn(...);
>     std::string path(pn.to_string(false)); // what does "false" mean here?
>
>With my suggestion, that statement looks like this:
>
>     std::string path(pn.to_string(pathname::no_tail));
>
>Change Pathname::Drive() to "volume."  That handles returning machine and
>share name for UNC filesystems, "/" for Unix filesystems, D:\ for FAT
>filesystems, etc.
>
>Change Pathname::Directory() to "path."  "Directory" is singular and so
>doesn't account for the multiple directories that might comprise that
>portion of the pathname.
>
>Change Pathname::Empty() to "is_empty."
>
>I don't think Exists() should be in Pathname.  One should use a separate
>class or set of functions for filesystem interactions.
>
>Pathname::MakeAbsolute() should take the "current directory" as a parameter.
>Not only does that do what your version does, if the caller supplies the
>real current directory, but it also allows the caller to convert a relative
>path into an absolute path based upon any path of their choice.  This is
>applicable for situations in which the program is given a relative path
>based upon the user's home directory or some data repository.
>
>Pathname::MakeRelative() says that the parameter defaults to the cwd, and
>yet the parameter has no default value.  Nevertheless, I wouldn't change the
>signature; just its description.  IOW, as with MakeAbsolute(), it should be
>up to the caller to provide the cwd, if that's what they want.

In retrospect, are MakeAbsolute or MakeRelative similar to Exists in that 
they should be handled outside the class?

>Pathname::Split() should not return a vector.  Instead, make it a member
>template that takes an output iterator.  I don't think the parameter is
>necessary.  The caller can skip the first string if they don't want the
>volume.  If you opt to keep that parameter, then create an enumerated type
>to control the behavior.

The parameter decides whether the volume should be included in the first 
parameter or not. Sometimes it is helpful to get the first part as 
"c:/data" instead of "c:" and "/data". I agree with the iterator though. 
I'll look into it.

>The inclusion of Pathname::IsUnc() and sSupportsUncNames bother me.  These
>are filesystem-specific.  I think the correct thing to do is to use a
>FileSystem policy class that subsumes "Case" and "Sep," and then such policy
>classes can provide some unique filesystem-specific functionality, like
>is_unc(), that aren't applicable to most filesystems.

This is a good idea, I'll look into it.

>While I did delve into some of the implementation issues as they had bearing
>on the interface, I didn't study your implementation itself.  I'll leave
>that to you, at least for now.

Thank you for your input.

Jason