Add implementations of various extension related methods (derived from file_u...

Add implementations of various extension related methods (derived from file_util):
Extension, RemoveExtension, InsertBeforeExtension, ReplaceExtension

I didn't reimplement the old file_util ones since they actually modify the FilePath in place, which isn't the style of the rest of the FilePath methods.  I'll file a cleanup bug after this for callers to switch to the new methods.


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

[Mark Mentovai, 2009-01-07 22:11:36]

http://codereview.chromium.org/17243/diff/213/216
File base/file_path.cc (right):

http://codereview.chromium.org/17243/diff/213/216#newcode144
Line 144: FilePath::StringType file_name = BaseName().value();
Again with the excess qualification...

I won't mention this again, but I see other occurrences after this too.

http://codereview.chromium.org/17243/diff/213/216#newcode154
Line 154: file_name, last_dot+1);
This is hard-ish to read because it looks like last_dot+1 (which incidentally
needs space around the +) is part of the ternary.

It's not part of the ternary, and it's also weird to use last_dot + 1 as the pos
argument in the "" case.

This might be better restructured as a traditional if-type conditional.

http://codereview.chromium.org/17243/diff/213/216#newcode160
Line 160: // kExtensionSeparator actually appeared in the last path component.
Extension() doesn't differentiate between extensionless names ("file") and names
that end in the separator ("file."), returning "" as the extension for both. 
But for RemoveExtension, it seems like we should take off the trailing dot if
there is one.

We should test behavior here in the unit tests, too.

http://codereview.chromium.org/17243/diff/213/216#newcode174
Line 174: if (*(path_.end() - 1) == kExtensionSeparator[0]) {
If the FilePath has trailing separators, Extension and RemoveExtension
effectively ignore them - they behave the same for input "/dir/file.ext" and
"/dir/file.ext///", and they behave the same for input "/." and "/.//".  The
special-casing for . and .. you're putting here is good, but this line means
that you'll catch "/.." as .. and hit the early return here but you won't catch
"/../" and will continue processing, resulting in something like "/. (1)." being
returned.

http://codereview.chromium.org/17243/diff/213/216#newcode179
Line 179: return FilePath(path_);
Do we want to return this FilePath if we couldn't insert the new text?  Should
we just return an empty FilePath?

http://codereview.chromium.org/17243/diff/213/216#newcode185
Line 185: FilePath::StringType str = no_ext.value();
I'd just save a line and a temporary here and do:

StringType str = RemoveExtension().value();

http://codereview.chromium.org/17243/diff/213/216#newcode200
Line 200: if (*(path_.end() - 1) == kExtensionSeparator[0]) {
As earlier.

http://codereview.chromium.org/17243/diff/213/216#newcode215
Line 215: if (extension.find(kExtensionSeparator) != 0)
We should look for kExtensionSeparator in the first position.  If you have
FilePath("myarchive").ReplaceExtension("tar.bz2"), it should behave the same as
FilePath("myarchive").ReplaceExtension(".tar.bz2"), shouldn't it?

http://codereview.chromium.org/17243/diff/213/216#newcode265
Line 265: bool FilePath::Contains(const FilePath &other) const {
Crud, I don't know when this was added, but it should come out (or move to
file_util).  FilePaths should never touch the disk.

(...or depend on file_util...?)

http://codereview.chromium.org/17243/diff/213/217
File base/file_path.h (right):

http://codereview.chromium.org/17243/diff/213/217#newcode155
Line 155: StringType Extension() const;
I'm glad to see you made these return StringType and not FilePath.

http://codereview.chromium.org/17243/diff/213/217#newcode169
Line 169: FilePath InsertBeforeExtension(const FilePath::StringType& suffix)
const;
This doesn't need the FilePath:: qualifier on StringType, does it?  Leaving the
qualifier intact may break gcc builds.

http://codereview.chromium.org/17243/diff/213/217#newcode174
Line 174: FilePath ReplaceExtension(const FilePath::StringType& extension)
const;
Same.

http://codereview.chromium.org/17243/diff/213/215
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/17243/diff/213/215#newcode1
Line 1: // Copyright (c) 2008 The Chromium Authors. All rights reserved.
Also, please add cases to cover some of the other cornery things I mentioned in
file_path.cc.

http://codereview.chromium.org/17243/diff/213/215#newcode414
Line 414: ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &base_dir));
Why are we doing this?  We don't care about what's on the disk in FilePath, and
it seems like you're just using it to get a dummy name.  You might as well
choose a static dummy name.

http://codereview.chromium.org/17243/diff/213/215#newcode429
Line 429: static const struct InsertBeforeExtensionCase {
Don't define a new struct for this, that's what BinaryTestData in this file is
all about.

If you put the struct outside of the InsertBeforeExtension test body just to get
the type not be anonymous for arraysize purposes, please move the struct back
into the test body.

http://codereview.chromium.org/17243/diff/213/215#newcode434
Line 434: {FPL(""), FPL(""), FPL("")},
Style in this file is { FPL("") } with spaces just inside the curlies, and with
the columns lined up when possible.

http://codereview.chromium.org/17243/diff/213/215#newcode450
Line 450: #if defined(OS_WIN)
Should be #if defined(FILE_PATH_USES_WIN_SEPARATORS).  These special defines are
good for testing on other systems.

http://codereview.chromium.org/17243/diff/213/215#newcode464
Line 464: #elif defined(OS_POSIX)
I don't think there's anything POSIX-specific about this section.  These tests
(or almost all of them) should work perfectly when FILE_PATH_USES_WIN_SEPARATORS
is defined, because our FilePath for Windows understands both separators.

http://codereview.chromium.org/17243/diff/213/215#newcode485
Line 485: EXPECT_EQ(kInsertBeforeExtension[i].result, result.value());
These loops suck when tests fail.  Look at some of the other EXPECT_EQs in this
file, which print things like the value of i and the inputs.

http://codereview.chromium.org/17243/diff/213/215#newcode490
Line 490: static const struct extension_case {
As above, UnaryTestData this time.

http://codereview.chromium.org/17243/diff/213/215#newcode494
Line 494: #if defined(OS_WIN)
FILE_PATH_USES_WIN_SEPARATORS again.

http://codereview.chromium.org/17243/diff/213/215#newcode526
Line 526: static const struct ReplaceExtensionCase {
And BinaryTestData here too.

http://codereview.chromium.org/17243/diff/213/215#newcode547
Line 547: #if defined(OS_WIN)
Fix #ifdef.

http://codereview.chromium.org/17243/diff/213/215#newcode561
Line 561: 
Trailing newline.

[Erik does not do reviews, 2009-01-08 00:39:44]

new snapshot uploaded

http://codereview.chromium.org/17243/diff/213/216
File base/file_path.cc (right):

http://codereview.chromium.org/17243/diff/213/216#newcode154
Line 154: file_name, last_dot+1);
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> This is hard-ish to read because it looks like last_dot+1 (which incidentally
> needs space around the +) is part of the ternary.
> 
> It's not part of the ternary, and it's also weird to use last_dot + 1 as the
pos
> argument in the "" case.
> 
> This might be better restructured as a traditional if-type conditional.

Agree.  I was a bit too lenient with my copy and paste from file_util.

http://codereview.chromium.org/17243/diff/213/216#newcode160
Line 160: // kExtensionSeparator actually appeared in the last path component.
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Extension() doesn't differentiate between extensionless names ("file") and
names
> that end in the separator ("file."), returning "" as the extension for both. 
> But for RemoveExtension, it seems like we should take off the trailing dot if
> there is one.
> 
> We should test behavior here in the unit tests, too.

I'm not actually sure what the right thing is to do here.  In Windows, trailing
.'s are ignored.  However, in POSIX, you can have filenames like "foo.",
"foo..", etc.  So the question is what does RemoveExtension do as a rule?  It
would be nice if it was cleanly reversible, but I don't see a clean way to do
that in these cases unless I just treat trailing .'s as if there's no extension.

http://codereview.chromium.org/17243/diff/213/216#newcode174
Line 174: if (*(path_.end() - 1) == kExtensionSeparator[0]) {
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> If the FilePath has trailing separators, Extension and RemoveExtension
> effectively ignore them - they behave the same for input "/dir/file.ext" and
> "/dir/file.ext///", and they behave the same for input "/." and "/.//".  The
> special-casing for . and .. you're putting here is good, but this line means
> that you'll catch "/.." as .. and hit the early return here but you won't
catch
> "/../" and will continue processing, resulting in something like "/. (1)."
being
> returned.

Good catch.  I now test against BaseName() which strips trailing separators.

http://codereview.chromium.org/17243/diff/213/216#newcode179
Line 179: return FilePath(path_);
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Do we want to return this FilePath if we couldn't insert the new text?  Should
> we just return an empty FilePath?

Seems reasonable.  This was a copy of the old behavior, but it doesn't look like
any existing code is using relative directories or user-supplied paths, so I
think it's OK to change.  I made the same change in ReplaceExtension.

http://codereview.chromium.org/17243/diff/213/216#newcode215
Line 215: if (extension.find(kExtensionSeparator) != 0)
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> We should look for kExtensionSeparator in the first position.  If you have
> FilePath("myarchive").ReplaceExtension("tar.bz2"), it should behave the same
as
> FilePath("myarchive").ReplaceExtension(".tar.bz2"), shouldn't it?

Yes.  This is exactly the intent of this code (and some test cases).  Is there a
case I missed here?

http://codereview.chromium.org/17243/diff/213/216#newcode265
Line 265: bool FilePath::Contains(const FilePath &other) const {
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Crud, I don't know when this was added, but it should come out (or move to
> file_util).  FilePaths should never touch the disk.
> 
> (...or depend on file_util...?)

Oops.  Looks like I reviewed this change.  I'll remove this in a follow-on
changelist (I have another pending change that would conflict).

http://codereview.chromium.org/17243/diff/213/215
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/17243/diff/213/215#newcode414
Line 414: ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &base_dir));
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Why are we doing this?  We don't care about what's on the disk in FilePath,
and
> it seems like you're just using it to get a dummy name.  You might as well
> choose a static dummy name.

yep.  It just avoided an #ifdef.  I guess I don't need an absolute path, so I'll
do it that way.

http://codereview.chromium.org/17243/diff/213/215#newcode429
Line 429: static const struct InsertBeforeExtensionCase {
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Don't define a new struct for this, that's what BinaryTestData in this file is
> all about.
> 
> If you put the struct outside of the InsertBeforeExtension test body just to
get
> the type not be anonymous for arraysize purposes, please move the struct back
> into the test body.

ah, didn't notice that.  This code was mostly lifted from another test that
didn't have that struct.  fixed.

http://codereview.chromium.org/17243/diff/213/215#newcode434
Line 434: {FPL(""), FPL(""), FPL("")},
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Style in this file is { FPL("") } with spaces just inside the curlies, and
with
> the columns lined up when possible.

I didn't see an easy way to line up the columns with these.  There are a handful
of long paths that make it not work out.

http://codereview.chromium.org/17243/diff/213/215#newcode450
Line 450: #if defined(OS_WIN)
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> Should be #if defined(FILE_PATH_USES_WIN_SEPARATORS).  These special defines
are
> good for testing on other systems.

OK.

http://codereview.chromium.org/17243/diff/213/215#newcode464
Line 464: #elif defined(OS_POSIX)
On 2009/01/07 22:11:36, Mark Mentovai wrote:
> I don't think there's anything POSIX-specific about this section.  These tests
> (or almost all of them) should work perfectly when
FILE_PATH_USES_WIN_SEPARATORS
> is defined, because our FilePath for Windows understands both separators.

That's nice.  That definitely improves the coverage a bit more.

[Mark Mentovai, 2009-01-08 18:04:20]

http://codereview.chromium.org/17243/diff/26/29
File base/file_path.cc (right):

http://codereview.chromium.org/17243/diff/26/29#newcode159
Line 159: if (ext.empty())
>I'm not actually sure what the right thing is to do here.  In Windows,
>trailing .'s are ignored.  However, in POSIX, you can have filenames
>like "foo.", "foo..", etc.  So the question is what does RemoveExtension
>do as a rule?  It would be nice if it was cleanly reversible, but I
>don't see a clean way to do that in these cases unless I just treat
>trailing .'s as if there's no extension.


If we don't remove the trailing dot in RemoveExtension, then
FilePath("file.").ReplaceExtension("txt") will return "file..txt", which I think
is wrong.  (Do you agree?)  If so, it seems to me that the expectation of
RemoveExtension is that it will get rid of a lone trailing dot.

If you fix this, it would be good to add this to the ReplaceExtension test data:

{ { FPL("file."), FPL("txt") }, FPL("file.txt") }

http://codereview.chromium.org/17243/diff/26/29#newcode175
Line 175: if (*(base.end() - 1) == kExtensionSeparator[0]) {
I don't know if this is safe.  If BaseName().value() is an empty string (which
might happen in the unlikely but possible case of an empty FilePath), base.end()
== base.begin(), so the -1 causes this to dereference something before the
beginning of the string.  Is that OK?

http://codereview.chromium.org/17243/diff/26/29#newcode197
Line 197: if (*(base.end() - 1) == kExtensionSeparator[0]) {
As above.

http://codereview.chromium.org/17243/diff/26/29#newcode210
Line 210: if (extension.find(kExtensionSeparator) != 0)
Oops, this code misled me.  I misread the "!= 0" as "!= StringType::npos".

I think that this is misleading as written because we don't actually care about
finding the first dot in the extension (if any), we really just care about
whether there's a dot in the first character position.  Checking the first
character position is cheaper, too.  Do you agree, or is there something I
missed that makes it important to use find here?

http://codereview.chromium.org/17243/diff/26/28
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/17243/diff/26/28#newcode428
Line 428: { { FPL(""), FPL("") }, FPL("") },
I think we should line up the short cases here, and allow the really long lines
to wrap (they're already wrapped).

http://codereview.chromium.org/17243/diff/26/28#newcode490
Line 490: ", path: " << path.value() << ", insert: " << cases[i].inputs[1];
Nit: continuation line doesn't use a two-space indent, use four spaces or line
it up with something meaningful from the previous line (not really possible
here).

http://codereview.chromium.org/17243/diff/26/28#newcode527
Line 527: ", path: " << path.value();
Here too.

http://codereview.chromium.org/17243/diff/26/28#newcode561
Line 561: ", path: " << path.value() << ", replace: " << cases[i].inputs[i];
This one's a good example.

[Erik does not do reviews, 2009-01-09 01:16:12]

http://codereview.chromium.org/17243/diff/26/29
File base/file_path.cc (right):

http://codereview.chromium.org/17243/diff/26/29#newcode159
Line 159: if (ext.empty())
On 2009/01/08 18:04:20, Mark Mentovai wrote:
> >I'm not actually sure what the right thing is to do here.  In Windows,
> >trailing .'s are ignored.  However, in POSIX, you can have filenames
> >like "foo.", "foo..", etc.  So the question is what does RemoveExtension
> >do as a rule?  It would be nice if it was cleanly reversible, but I
> >don't see a clean way to do that in these cases unless I just treat
> >trailing .'s as if there's no extension.
> 
> 
> If we don't remove the trailing dot in RemoveExtension, then
> FilePath("file.").ReplaceExtension("txt") will return "file..txt", which I
think
> is wrong.  (Do you agree?)  If so, it seems to me that the expectation of
> RemoveExtension is that it will get rid of a lone trailing dot.
> 
> If you fix this, it would be good to add this to the ReplaceExtension test
data:
> 
> { { FPL("file."), FPL("txt") }, FPL("file.txt") }

I'm OK implementing it this way as I think this is an odd edge case anyway. 
However, I don't think you understood my concerns, so let me try to articulate
them one more time before I make this change.

(1) What do we do with multiple ending '.' characters?
RemoveExtension("file..") == "file."?

(2) Extension() returns "" in these cases, but RemoveExtension() does something.
 This means that this code doesn't work:

if (!path.Extension().empty()) {
  path = path.RemoveExtension();
} else {
  ASSERT(path == path.RemoveExtension());
}

I'd agree that it's not great code, but reading it you'd expect it to work, and
it doesn't after your change.

The cleanest way to implement this would be to have Extension include the
leading "." and to say that it returns either "" or a string that starts with
exactly one ".".  That way you could have code like this which would always
work:

new_path = path.RemoveExtension().Append(path.Extension());
ASSERT(new_path == path);

Given that I didn't change the file_util functions to use these new FilePath
methods, we could make this change cleanly without breaking anyone and just put
a note so that people who switch to the new code know that it's a bit different.
 I'm not sure if users of the API will find this more confusing though.

I made all of the other changes you suggested and uploaded.

[Mark Mentovai, 2009-01-09 18:21:44]

LGTM!