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

From: Stewart, Robert (stewart_at_[hidden])
Date: 2002-01-25 11:50:30

From:	Jason Stewart [SMTP:res0054p_at_[hidden]]
> 
> > > I have reworked the class that I submitted earlier (its not complete
yet,
> >I
> > > just wanted to see what people thought about the interface).
> > > http://stewart.simwright.net/samples/Pathname.h
> >
> >I don't know.  I get a 404: Not Found on that URL.
> 
> 
> Sorry, that should have been:
> 
> http://stewart.simwright.net/sample/Pathname.h

That works.

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

- Consider trailing const.
- Use all lower case identifiers with underscores, except for the template
parameters.  That means "Pathname" should be "pathname" and
"ForwardSeparator" should be "forward_separator," etc.

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.)  (BTW, I find it curious
that you used a leading lower case letter for the policy class mfs, and a
leading uppercase letter for Pathname's mfs.  Both should change to all
lower case with underscores, of course.)

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

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

    return c == '/';

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.

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

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
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.

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 comparison operators don't indicate their interaction with the Case
template parameter, nor do the path manipulation functions indicate their
interaction with the Sep template parameter.

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.

Pathname::path_ and dir_ should *not* be protected, they should be private.
Since Pathname does not have a virtual dtor, there should be nothing in a
protected section.

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.

Rob
Susquehanna International Group, LLP
http://www.sig.com