Move PathComponents from file_util to FilePath, add FilePath::IsParent()

Move PathComponents from file_util to FilePath, add FilePath::IsParent()

r=erikkay,mark

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

[Erik does not do reviews, 2009-06-23 03:41:45]

http://codereview.chromium.org/145026/diff/2020/3001
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/2020/3001#newcode76
Line 76: void FilePath::PathComponents(const FilePath& path,
for future reference (since I'd prefer you change this away from static anyway),
all static method should have a // static comment above their implementation.

http://codereview.chromium.org/145026/diff/2020/3001#newcode85
Line 85: path_str.find_first_of(FilePath::kSeparators);
you need to handle the possibility of a leading drive letter.  See DirName and
BaseName below.

http://codereview.chromium.org/145026/diff/2020/3001#newcode101
Line 101: components->push_back(FilePath::StringType(path_str, start));
I think for any path where the while loop does nothing, this will wind up
duplicating the one path component.

http://codereview.chromium.org/145026/diff/2020/3001#newcode110
Line 110: if (parent_components.size() > child_components.size())
>=?  I take it a path isn't its own parent.

http://codereview.chromium.org/145026/diff/2020/3001#newcode120
Line 120: child_comp++;
use ++parent_comp; and ++child_comp;  See effective C++ for why pre-increment is
always preferred to post-increment.

http://codereview.chromium.org/145026/diff/2020/3002
File base/file_path.h (right):

http://codereview.chromium.org/145026/diff/2020/3002#newcode146
Line 146: static void PathComponents(const FilePath& path,
why a static?  shouldn't this just be an instance method?

http://codereview.chromium.org/145026/diff/2020/3003
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/145026/diff/2020/3003#newcode384
Line 384: {FPL("e:/foo"), FPL("e:|foo")},
need more test cases.  add "/", "c:", "c:/", "foo", "" and "foo/bar" to your
test cases at least.

http://codereview.chromium.org/145026/diff/2020/3003#newcode408
Line 408: {FPL("/foo/bar"), FPL("/foo/bar/baz"), true},
more cases, add an equality test case, a case where number of components is
greater or equal in the parent than child, a case where parent ends with the
path separator.

http://codereview.chromium.org/145026/diff/2020/3003#newcode419
Line 419: #endif // FILE_PATH_USES_DRIVE_LETTERS
two spaces before //

[Mark Mentovai, 2009-06-23 03:48:04]

I have some comments in addition to what Erik said.  You should address
everything he mentioned too, and I'll make a closer pass when you've gotten
these things taken care of.

http://codereview.chromium.org/145026/diff/2020/3001
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/2020/3001#newcode81
Line 81: 
Shouldn't you clear out |components| here before you do anything else to it?

http://codereview.chromium.org/145026/diff/2020/3001#newcode88
Line 88: if (end == start) {
Seems like this |if| and the |while| that follows it ought to be consolidated.

[rafaelw, 2009-06-23 23:03:40]

I've changed the implementation of GetComponents per my discussion with Mark.

http://codereview.chromium.org/145026/diff/2020/3001
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/2020/3001#newcode76
Line 76: void FilePath::PathComponents(const FilePath& path,
On 2009/06/23 03:41:45, Erik Kay wrote:
> for future reference (since I'd prefer you change this away from static
anyway),
> all static method should have a // static comment above their implementation.

Ok.

http://codereview.chromium.org/145026/diff/2020/3001#newcode81
Line 81: 
On 2009/06/23 03:48:04, Mark Mentovai wrote:
> Shouldn't you clear out |components| here before you do anything else to it?

Done.

http://codereview.chromium.org/145026/diff/2020/3001#newcode85
Line 85: path_str.find_first_of(FilePath::kSeparators);
On 2009/06/23 03:41:45, Erik Kay wrote:
> you need to handle the possibility of a leading drive letter.  See DirName and
> BaseName below.

Done.

http://codereview.chromium.org/145026/diff/2020/3001#newcode88
Line 88: if (end == start) {
On 2009/06/23 03:48:04, Mark Mentovai wrote:
> Seems like this |if| and the |while| that follows it ought to be consolidated.

Done.

http://codereview.chromium.org/145026/diff/2020/3001#newcode101
Line 101: components->push_back(FilePath::StringType(path_str, start));
On 2009/06/23 03:41:45, Erik Kay wrote:
> I think for any path where the while loop does nothing, this will wind up
> duplicating the one path component.

Done.

http://codereview.chromium.org/145026/diff/2020/3001#newcode110
Line 110: if (parent_components.size() > child_components.size())
On 2009/06/23 03:41:45, Erik Kay wrote:
> >=?  I take it a path isn't its own parent.

Done.

http://codereview.chromium.org/145026/diff/2020/3001#newcode120
Line 120: child_comp++;
On 2009/06/23 03:41:45, Erik Kay wrote:
> use ++parent_comp; and ++child_comp;  See effective C++ for why pre-increment
is
> always preferred to post-increment.

Done.

http://codereview.chromium.org/145026/diff/2020/3002
File base/file_path.h (right):

http://codereview.chromium.org/145026/diff/2020/3002#newcode146
Line 146: static void PathComponents(const FilePath& path,
On 2009/06/23 03:41:45, Erik Kay wrote:
> why a static?  shouldn't this just be an instance method?

Done.

http://codereview.chromium.org/145026/diff/2020/3003
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/145026/diff/2020/3003#newcode384
Line 384: {FPL("e:/foo"), FPL("e:|foo")},
On 2009/06/23 03:41:45, Erik Kay wrote:
> need more test cases.  add "/", "c:", "c:/", "foo", "" and "foo/bar" to your
> test cases at least.

Done.

http://codereview.chromium.org/145026/diff/2020/3003#newcode408
Line 408: {FPL("/foo/bar"), FPL("/foo/bar/baz"), true},
On 2009/06/23 03:41:45, Erik Kay wrote:
> more cases, add an equality test case, a case where number of components is
> greater or equal in the parent than child, a case where parent ends with the
> path separator.
> 

Done.

http://codereview.chromium.org/145026/diff/2020/3003#newcode419
Line 419: #endif // FILE_PATH_USES_DRIVE_LETTERS
On 2009/06/23 03:41:45, Erik Kay wrote:
> two spaces before //

Done.

[Erik does not do reviews, 2009-06-23 23:13:45]

LGTM

thanks for doing this cleanup!

[Mark Mentovai, 2009-06-23 23:17:54]

I'm leaving in a sec, but I wanted to share my comments on the test with you
first.

http://codereview.chromium.org/145026/diff/3010/2029
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/145026/diff/3010/2029#newcode378
Line 378: static const struct PathComponentsCase {
Don't define a new struct, just reuse UnaryTestData.

Don't declare the test data in the global scope, put it inside the TEST_F.

Please follow the other examples in this file.

http://codereview.chromium.org/145026/diff/3010/2029#newcode387
Line 387: {FPL("foo"), FPL("foo")},
I'd like to see a lot more test cases here.  Multiple trailing slashes, three
(instead of just two) leading slashes, backslashes (alone and mixed with
slashes) if USES_WIN_SEPARATORS is true, relative paths beginning with . and ..,
and an empty string.

http://codereview.chromium.org/145026/diff/3010/2029#newcode414
Line 414: static const struct IsParentCase {
Define BinaryBooleanTestData above and use it here.

Don't put this in the global scope, put it in the TEST_F.

http://codereview.chromium.org/145026/diff/3010/2029#newcode431
Line 431: {FPL("c:\\foo\\bar"), FPL("c:\\foo\\bar\\baz"), true},
FILE_PATH_USES_DRIVE_LETTERS is not the way to check if you can use backslashes.
 There's a separate macro you need for that.  As these tests are written, you'd
need to check both macros.

Consider extending these test cases too in some of the ways I mentioned above.

[rafaelw, 2009-06-24 02:05:58]

http://codereview.chromium.org/145026/diff/3010/2029
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/145026/diff/3010/2029#newcode378
Line 378: static const struct PathComponentsCase {
On 2009/06/23 23:17:54, Mark Mentovai wrote:
> Don't define a new struct, just reuse UnaryTestData.
> 
> Don't declare the test data in the global scope, put it inside the TEST_F.
> 
> Please follow the other examples in this file.

Done.

http://codereview.chromium.org/145026/diff/3010/2029#newcode387
Line 387: {FPL("foo"), FPL("foo")},
On 2009/06/23 23:17:54, Mark Mentovai wrote:
> I'd like to see a lot more test cases here.  Multiple trailing slashes, three
> (instead of just two) leading slashes, backslashes (alone and mixed with
> slashes) if USES_WIN_SEPARATORS is true, relative paths beginning with . and
..,
> and an empty string.

Done.

http://codereview.chromium.org/145026/diff/3010/2029#newcode414
Line 414: static const struct IsParentCase {
On 2009/06/23 23:17:54, Mark Mentovai wrote:
> Define BinaryBooleanTestData above and use it here.
> 
> Don't put this in the global scope, put it in the TEST_F.

Done.

http://codereview.chromium.org/145026/diff/3010/2029#newcode431
Line 431: {FPL("c:\\foo\\bar"), FPL("c:\\foo\\bar\\baz"), true},
On 2009/06/23 23:17:54, Mark Mentovai wrote:
> FILE_PATH_USES_DRIVE_LETTERS is not the way to check if you can use
backslashes.
>  There's a separate macro you need for that.  As these tests are written,
you'd
> need to check both macros.
> 
> Consider extending these test cases too in some of the ways I mentioned above.

Done.

[Mark Mentovai, 2009-06-24 16:11:25]

I have a couple of nits about using existing constants and that sort of thing,
but by and large, this looks good.

http://codereview.chromium.org/145026/diff/3016/2035
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/3016/2035#newcode94
Line 94: base.value() != FILE_PATH_LITERAL("\\") &&
You shouldn't be checking for backslashes unless FILE_PATH_USES_WIN_SEPARATORS
is true.

This would be better if instead of having a new hard-code here, you looked at
kSeparators.  Each character in kSeparators is a separator, so you should just
be checking to see if any character in kSeparators appears either singly or in
duplicate.

http://codereview.chromium.org/145026/diff/3016/2035#newcode102
Line 102: if (base.value() != FILE_PATH_LITERAL(".") &&
Use kCurrentDirectory.

http://codereview.chromium.org/145026/diff/3016/2035#newcode114
Line 114: ret_val.rend());
Can you move this over so that this ret_val lines up with the one on the
preceding line?  I'd find that easier to read.

http://codereview.chromium.org/145026/diff/3016/2035#newcode120
Line 120: this->GetComponents(&parent_components);
this-> is unnecessary.  Just GetComponents.

http://codereview.chromium.org/145026/diff/3016/2035#newcode132
Line 132: for (; parent_comp != parent_components.end();) {
This should be a "while" loop because you've castrated the initial and
end-of-loop statements.

http://codereview.chromium.org/145026/diff/3016/2036
File base/file_path.h (right):

http://codereview.chromium.org/145026/diff/3016/2036#newcode153
Line 153: // symlinks or directory navigation (e.g. ".."). A path is *NOT* it's
own
it's -> its

[rafaelw, 2009-06-24 17:56:59]

http://codereview.chromium.org/145026/diff/3016/2035
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/3016/2035#newcode94
Line 94: base.value() != FILE_PATH_LITERAL("\\") &&
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> You shouldn't be checking for backslashes unless FILE_PATH_USES_WIN_SEPARATORS
> is true.
> 
> This would be better if instead of having a new hard-code here, you looked at
> kSeparators.  Each character in kSeparators is a separator, so you should just
> be checking to see if any character in kSeparators appears either singly or in
> duplicate.

Done.

http://codereview.chromium.org/145026/diff/3016/2035#newcode102
Line 102: if (base.value() != FILE_PATH_LITERAL(".") &&
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> Use kCurrentDirectory.

Done.

http://codereview.chromium.org/145026/diff/3016/2035#newcode114
Line 114: ret_val.rend());
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> Can you move this over so that this ret_val lines up with the one on the
> preceding line?  I'd find that easier to read.

Done.

http://codereview.chromium.org/145026/diff/3016/2035#newcode120
Line 120: this->GetComponents(&parent_components);
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> this-> is unnecessary.  Just GetComponents.

Done.

http://codereview.chromium.org/145026/diff/3016/2035#newcode132
Line 132: for (; parent_comp != parent_components.end();) {
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> This should be a "while" loop because you've castrated the initial and
> end-of-loop statements.

Done.

http://codereview.chromium.org/145026/diff/3016/2036
File base/file_path.h (right):

http://codereview.chromium.org/145026/diff/3016/2036#newcode153
Line 153: // symlinks or directory navigation (e.g. ".."). A path is *NOT* it's
own
On 2009/06/24 16:11:25, Mark Mentovai wrote:
> it's -> its

Done.

[Mark Mentovai, 2009-06-24 17:59:43]

Great, this LGTM.  Thanks.

http://codereview.chromium.org/145026/diff/2052/3022
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/2052/3022#newcode73
Line 73: }  // namespace
Leave the blank line between the end of the function and the end of the
namespace.

[rafaelw, 2009-06-24 18:07:52]

http://codereview.chromium.org/145026/diff/2052/3022
File base/file_path.cc (right):

http://codereview.chromium.org/145026/diff/2052/3022#newcode73
Line 73: }  // namespace
On 2009/06/24 17:59:44, Mark Mentovai wrote:
> Leave the blank line between the end of the function and the end of the
> namespace.

Done.