Add a FilePath object

Add a FilePath object

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2838

[Mark Mentovai, 2008-10-01 14:24:35]

I should probably just check this in instead of hoarding it, I can make further
changes and start moving string-users over once it's in the tree.

[tony, 2008-10-01 16:03:48]

Do we need special handling for UNC paths on Windows?

Do we want to have logic in this class to disallow invalid path characters on
Windows (or null on posix?)


On 2008/10/01 14:24:35, Mark Mentovai wrote:
> I should probably just check this in instead of hoarding it, I can make
further
> changes and start moving string-users over once it's in the tree.

[brettw, 2008-10-01 16:43:15]

http://codereview.chromium.org/6025/diff/1/6
File base/file_path.cc (right):

http://codereview.chromium.org/6025/diff/1/6#newcode38
Line 38: if (last_separator == StringType::npos)  // was in the current
directory
Your comments should be complete sentences, dude.

http://codereview.chromium.org/6025/diff/1/7
File base/file_path.h (right):

http://codereview.chromium.org/6025/diff/1/7#newcode59
Line 59: FilePath BaseName() const;
This name is confusing to me. This will return foo.txt if the filename is
"c:\bar\foo.txt", right? Can can you have "File" somewhere in the path to make
it distinct from "Dir"?

[jungshik at Google, 2008-10-01 19:11:44]

http://codereview.chromium.org/6025/diff/1/6
File base/file_path.cc (right):

http://codereview.chromium.org/6025/diff/1/6#newcode21
Line 21: const int FilePath::kMax = MAX_PATH;
I guess you don't want to support a really long path (~ 32k)  on Windows because
not all Windows APIs support it(?). Do we need a comment to that effect here? 
http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx

http://codereview.chromium.org/6025/diff/1/6#newcode154
Line 154: if (path_.length() >= 2 && path_[1] == ':' && _iswalpha_l(path_[0],
NULL)) {
Just curious (because I couldn't find a reference to passing NULL to *_l
functions in the MSDN), is NULL equivalent to 'C' locale for *_l functions?

http://codereview.chromium.org/6025/diff/1/5
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/6025/diff/1/5#newcode285
Line 285: { FPL("?:\\a"), false },
Can you add a test with FPL("\x00C5:\\a") here?  U+00C5 is 'A with a ring
above'.

[Erik does not do reviews, 2008-10-01 19:40:07]

http://codereview.chromium.org/6025/diff/1/7
File base/file_path.h (right):

http://codereview.chromium.org/6025/diff/1/7#newcode57
Line 57: // Returns a FilePath corresponding to the file named by this object,
should be something to the effect of "corresponding to the last path component
of this object, either a file or a directory"

http://codereview.chromium.org/6025/diff/1/7#newcode61
Line 61: // Returns a FilePath by appending a separator and the leaf component
to this
You mentioned that you didn't want to introduce 'leaf' earlier, and I agree.  I
think it's a bit confusing here.  Why not "path component"?

http://codereview.chromium.org/6025/diff/1/7#newcode66
Line 66: // and parent directories, if possible, and transforming all separator
this is a bit confusing.  I know you mean "removes '.' and '..'", but this makes
it sound like you're removing the current and parent components always rather
than just normalizing the special names.

http://codereview.chromium.org/6025/diff/1/7#newcode86
Line 86: FilePath MakeAbsolute() const;
I think we should get rid of this.  Dealing with current working directory is
notoriously error prone.  It's useful for scripts, but I'd rather that we made
this object enforce that it only deals with absolute paths.

http://codereview.chromium.org/6025/diff/1/7#newcode89
Line 89: static FilePath CurrentDirectory();
If we can get rid of MakeAbsolute, do we really need this?  Just having it
around will likely lead to it being used. In previous code reviews, Darin had
asked for us to specifically try to remove references to current working
directory.

http://codereview.chromium.org/6025/diff/1/7#newcode98
Line 98: // separator, and if it corresponds to the current directory, strips
that
Can you just say 'kCurrentDirectory' here?  I think it would be more clear since
"current directory" reads like "getcwd" to me.

http://codereview.chromium.org/6025/diff/1/7#newcode105
Line 105: // separator, and if it corresponds to the parent directory, strips
that path
again, if you mean kParentDirectory, use that.

[cpu_(ooo_6.6-7.5), 2008-10-02 01:34:13]

Have we considered in windows just calling down to
the shell path api (in Shlwapi.dll)
http://msdn.microsoft.com/en-us/library/bb773559(VS.85).aspx

http://codereview.chromium.org/6025/diff/48/238
File base/file_path.cc (right):

http://codereview.chromium.org/6025/diff/48/238#newcode21
Line 21: const int FilePath::kMax = 32768;
The 32k limit is per path component, that is, you actually can have a windows
path longer than that. It think is best to remove this limit and in fact I don't
think it is used for anything right now.

http://codereview.chromium.org/6025/diff/48/238#newcode230
Line 230: size_t length = GetCurrentDirectory(0, NULL);
Race condition here. GetCurrentDirectory is process wide. Any thread can change
it at any time.

[rvargas (doing something else), 2008-10-02 06:18:18]

I wonder what's the actual use case for Normalize (and by extension
HasDriveLetter and IsAbsolute). It looks like the type of general purpose
functionality useful for scripts, but I don't recall any place where we need
something like that, and it looks like a complete implementation has a lot of
details to take care of.

I would consider more important to have code that actually just detects when a
path has embedded previous directory escapes, because it may come from some
somebody trying to get away from the folder where it should be writing, instead
of just making that conversion in the first place. Maybe we should always deal
with absolute paths and treat other cases as errors.

http://codereview.chromium.org/6025/diff/51/252
File base/file_path.cc (right):

http://codereview.chromium.org/6025/diff/51/252#newcode10
Line 10: const FilePath::CharType FilePath::kSeparators[] =
FILE_PATH_LITERAL("\\/");
A forward slash is not a valid separator on windows and I see no reason at all
to consider it as such.

http://codereview.chromium.org/6025/diff/51/252#newcode60
Line 60: new_path.path_ = kCurrentDirectory;
I find it highly surprising to receive "." as the directory component of a path
like "my_file.txt". That would mean that  the first thing that I would have to
do after calling this code is to compare against that special case because most
likely I will not be able to do anything with ".". This seems just an extension
of the "current directory" problem already mentioned.

http://codereview.chromium.org/6025/diff/51/252#newcode176
Line 176: if (path_.length() >= 2 && path_[1] == L':' &&
the drive letter may not be the first character for a valid path (for instance
\\?\c:\some is valid), but maybe I'm being too picky.

http://codereview.chromium.org/6025/diff/51/253
File base/file_path.h (right):

http://codereview.chromium.org/6025/diff/51/253#newcode77
Line 77: StringType::size_type HasDriveLetter() const;
nit: this name suggest a bool return value, not an index into the name.

[jungshik at Google, 2008-10-02 17:18:47]

http://codereview.chromium.org/6025/diff/211/18
File base/file_path.cc (right):

http://codereview.chromium.org/6025/diff/211/18#newcode21
Line 21: const int FilePath::kMax = 32768;
Sorry I was not clear (it's not 32ki but 32k=32000. Even that is approximaate
according to MSDN. I don't know why). The MSDN page I referred to has the
following. An extended path needs to start with "\\?\" prefix. I don't see any
code dealing with that. For now, I guess we just have to go with MAX_PATH (and a
pretty short path length of 260). 


<quote>
The Windows API has many functions that also have Unicode versions to permit an
extended-length path for a maximum total path length of approximately 32,000
characters. This type of path is composed of components separated by
backslashes, each up to 255 characters in length. To specify an extended-length
path, use the "\\?\" prefix. For example, "\\?\D:\<very long path>". (The
characters < > are used here for visual clarity and cannot be part of a valid
path string.)

Note  The maximum path of 32,000 characters is approximate, because the "\\?\"
prefix may be expanded by the system at run time to a longer string, and this
expansion applies to the total length.
</quote>

http://codereview.chromium.org/6025/diff/211/17
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/6025/diff/211/17#newcode342
Line 342: { FPL("\x0143:/a"),  false },  // 'C' with extra bit set
Sorry for nitpicking :-) FPL("\x0143:/a") would be expanded to L"\x0143:/a" on
Windows, which means \x0143 is interpreted as U+0143 (Latin capital letter N
with acute accent). The same is true of \xB3 and \xC5 (they'd be U+00B3
(subscript 3) and U+00C5 (a with ring above) on Windows while on Linux, 0xB3 is
what you said. Platform-dependent comment ?  :-) Maybe, just moving C5 case
inside #if defined(OS_WIN) is better.

[darin (slow to review), 2008-10-02 18:14:19]

this looks really great to me.  some comments on the header file...

http://codereview.chromium.org/6025/diff/51/253
File base/file_path.h (right):

http://codereview.chromium.org/6025/diff/51/253#newcode4
Line 4: 
nit: please add some comments up here.

it would be good to mention that FilePath is not mutable.  any operation that
generates a new path, results in a new FilePath object being created.  this is a
good thing to mention so that people can know that they can use a FilePath
object from multiple threads without worrying about locking.

http://codereview.chromium.org/6025/diff/51/253#newcode27
Line 27: typedef StringType::value_type CharType;
googleurl does it the other way around by defining CharType within the #ifdefs,
and then outside doing this:

  typedef std::basic_string<UTF16Char> UTF16String;

i wonder if that doesn't help in some way.

http://codereview.chromium.org/6025/diff/51/253#newcode40
Line 40: FilePath() : path_() {}
nit: ": path_()" can be removed

http://codereview.chromium.org/6025/diff/51/253#newcode44
Line 44: void operator=(const FilePath& that) { path_ = that.path_; }
nit: normally operator= should return FilePath& so you can do "a = b = c;"

http://codereview.chromium.org/6025/diff/51/253#newcode47
Line 47: const StringType& path() const { return path_; }
nit: it seems like this will lead to code like this "path.path()", which seems a
bit awkward.  i think it would be better to do something like "path.value()"
(the string value of the path)

http://codereview.chromium.org/6025/diff/51/253#newcode70
Line 70: FilePath Normalize() const;
Q: does this also resolve symlinks?

http://codereview.chromium.org/6025/diff/51/253#newcode81
Line 81: // a separator character, or with two separator characters.  On other
nit: "On Posix platforms,..."

[darin (slow to review), 2008-10-03 16:51:50]

most excellent.  LGTM!

http://codereview.chromium.org/6025/diff/601/607
File base/file_path.h (right):

http://codereview.chromium.org/6025/diff/601/607#newcode42
Line 42: // objects.  The objects themselves are safe to share.
"share [between threads]" ?