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
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.
>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
>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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk