Add InternalStyle:rootLength to implement isAbsolute and rootPrefix.

Add InternalStyle:rootLength to implement isAbsolute and rootPrefix.

This is the first step at adding a few helper methods for improving
path package performance.

BUG=
R=nweiz@google.com, rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38966

[Anders Johnsen, 2014-08-04 08:17:11]

Benchmark:

  var sw = new Stopwatch()..start();
  files
      .map((e) {
        p.rootPrefix(e.path);
        p.isAbsolute(e.path);
      })
      .toList(growable: false);
  print(sw.elapsed);


of sdk files.

Before:
0:00:00.010588
0:00:00.003282
0:00:00.003264
0:00:00.002410
0:00:00.002209
0:00:00.002828
0:00:00.001967
0:00:00.001472
0:00:00.001444
0:00:00.001455


After:

0:00:00.003411
0:00:00.000527
0:00:00.000531
0:00:00.001435
0:00:00.000351
0:00:00.002707
0:00:00.000604
0:00:00.000031
0:00:00.000029
0:00:00.000030

I suspect the spikes are time used on optimizing.

[Bob Nystrom, 2014-08-04 17:17:32]

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
File pkg/path/lib/src/internal_style.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
pkg/path/lib/src/internal_style.dart:36: /// Returns the offset of the root
part. Returns 0 if the path is relative.
"Returns the offset" -> "The number of characters".

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/posix...
File pkg/path/lib/src/style/posix.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/posix...
pkg/path/lib/src/style/posix.dart:39: int length = rootLength(path);
int -> var, here and elsewhere. We don't type annotate locals in path (and in
most other packages for that matter).

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.dart
File pkg/path/lib/src/style/url.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:47: index = path.indexOf('/', index + 3);
Document this line please.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:42: int index = path.indexOf('\\', 2);
Document this and the next indexOf too.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:49: return 1;
Nit, but I would reverse this flow control to reduce the nesting:

if (path.length < 2|| path.codeUnitAt(1) != chars.BACKSLASH) return 1;

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:50: }
These could use some comments too.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:61: return getRelativeRoot(path);
This code is the same for all three styles, isn't it? Move it up to Style or put
in a mixin?

[nweiz, 2014-08-04 20:11:58]

I'd still very much like to see benchmark scripts included in the package.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
File pkg/path/lib/src/internal_style.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
pkg/path/lib/src/internal_style.dart:36: /// Returns the offset of the root
part. Returns 0 if the path is relative.
The style guide says the first sentence of doc comments should be its own
paragraph.

Also, explain what this does for relative roots.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.dart
File pkg/path/lib/src/style/url.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:39: return path.endsWith("://");
This isn't sufficient; it fails on "foo/bar://".

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:50: }
I believe this also fails on "foo/bar://".

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:47: return path.length;
I believe Windows considers "\\foo" and "\\foo\bar" to be equivalent to "\foo"
and "\foo\bar" respectively, rather than UNC paths.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:61: return getRelativeRoot(path);
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> This code is the same for all three styles, isn't it? Move it up to Style or
put
> in a mixin?

+1 for making it a method of InternalStyle.

[Anders Johnsen, 2014-08-05 08:57:14]

Please elaborate on the form, location and framework to be used for benchmarks.

I would normally use the benchmark harness, but I'm not sure it's okay to add as
dependency to the package?

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
File pkg/path/lib/src/internal_style.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
pkg/path/lib/src/internal_style.dart:36: /// Returns the offset of the root
part. Returns 0 if the path is relative.
On 2014/08/04 20:11:57, nweiz wrote:
> The style guide says the first sentence of doc comments should be its own
> paragraph.
> 
> Also, explain what this does for relative roots.

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/internal_st...
pkg/path/lib/src/internal_style.dart:36: /// Returns the offset of the root
part. Returns 0 if the path is relative.
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> "Returns the offset" -> "The number of characters".

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/posix...
File pkg/path/lib/src/style/posix.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/posix...
pkg/path/lib/src/style/posix.dart:39: int length = rootLength(path);
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> int -> var, here and elsewhere. We don't type annotate locals in path (and in
> most other packages for that matter).

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.dart
File pkg/path/lib/src/style/url.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:39: return path.endsWith("://");
On 2014/08/04 20:11:58, nweiz wrote:
> This isn't sufficient; it fails on "foo/bar://".

I see.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:47: index = path.indexOf('/', index + 3);
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> Document this line please.

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/url.d...
pkg/path/lib/src/style/url.dart:50: }
On 2014/08/04 20:11:58, nweiz wrote:
> I believe this also fails on "foo/bar://".

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:42: int index = path.indexOf('\\', 2);
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> Document this and the next indexOf too.

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:47: return path.length;
On 2014/08/04 20:11:58, nweiz wrote:
> I believe Windows considers "\\foo" and "\\foo\bar" to be equivalent to "\foo"
> and "\foo\bar" respectively, rather than UNC paths.

Surprisingly, this is not tested. Can you find some documentation behind this,
so I can change it accordingly?

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:49: return 1;
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> Nit, but I would reverse this flow control to reduce the nesting:
> 
> if (path.length < 2|| path.codeUnitAt(1) != chars.BACKSLASH) return 1;

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:50: }
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> These could use some comments too.

Done.

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:61: return getRelativeRoot(path);
On 2014/08/04 17:17:32, Bob Nystrom wrote:
> This code is the same for all three styles, isn't it? Move it up to Style or
put
> in a mixin?

Done.

[Bob Nystrom, 2014-08-05 15:47:43]

On 2014/08/05 08:57:14, Anders Johnsen wrote:
> Please elaborate on the form, location and framework to be used for
benchmarks.

I see you sent out a patch for this, so I guess you figured it out. :)

As far as I know, there isn't much of an established practice for benchmarks in
packages yet, so I'm up for anything.

> I would normally use the benchmark harness, but I'm not sure it's okay to add
as
> dependency to the package?

It would be a devDependency, not a regular one, but I'm otherwise fine with this
if you think it's worth it.

Cheers!

- bob

[Bob Nystrom, 2014-08-05 16:04:48]

LGTM on my end, but please get and LGTM from Natalie too.

- bob

[nweiz, 2014-08-05 20:22:27]

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:47: return path.length;
On 2014/08/05 08:57:14, Anders Johnsen wrote:
> On 2014/08/04 20:11:58, nweiz wrote:
> > I believe Windows considers "\\foo" and "\\foo\bar" to be equivalent to
"\foo"
> > and "\foo\bar" respectively, rather than UNC paths.
> 
> Surprisingly, this is not tested. Can you find some documentation behind this,
> so I can change it accordingly?

http://msdn.microsoft.com/en-us/library/gg465305.aspx

Looks like "\\foo" isn't a valid UNC path, but "\\foo\bar" is. We should
definitely add tests for these.

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
File pkg/path/lib/src/internal_style.dart (right):

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
pkg/path/lib/src/internal_style.dart:36: /// Returns the number of characters of
the root part. Returns 0 if the path
Nit: Looks like this is still a two-sentence first paragraph.

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
pkg/path/lib/src/internal_style.dart:50: /// Gets if [path] is root-relative.
"Gets if" -> "Returns whether"

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/style/u...
File pkg/path/lib/src/style/url.dart (right):

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/style/u...
pkg/path/lib/src/style/url.dart:39: return rootLength(path) > 0 &&
path.endsWith("://");
I believe this still breaks on "http://foo.com/bar://". I suggest:

    return rootLength(path) == path.length && path.endsWith("://");

[Anders Johnsen, 2014-08-06 07:05:16]

PTAL

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:47: return path.length;
On 2014/08/05 20:22:26, nweiz wrote:
> On 2014/08/05 08:57:14, Anders Johnsen wrote:
> > On 2014/08/04 20:11:58, nweiz wrote:
> > > I believe Windows considers "\\foo" and "\\foo\bar" to be equivalent to
> "\foo"
> > > and "\foo\bar" respectively, rather than UNC paths.
> > 
> > Surprisingly, this is not tested. Can you find some documentation behind
this,
> > so I can change it accordingly?
> 
> http://msdn.microsoft.com/en-us/library/gg465305.aspx
> 
> Looks like "\\foo" isn't a valid UNC path, but "\\foo\bar" is. We should
> definitely add tests for these.

But saying it's invalid doesn't indicate how we then should handle it in this
case. If I say '\\localhost' in start->run, it'll take me to a list of network
shares on my computer. I think we'll just have to treat it as the root in any
case. It's not, by any mean, a relative root. Thoughts?

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
File pkg/path/lib/src/internal_style.dart (right):

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
pkg/path/lib/src/internal_style.dart:36: /// Returns the number of characters of
the root part. Returns 0 if the path
On 2014/08/05 20:22:26, nweiz wrote:
> Nit: Looks like this is still a two-sentence first paragraph.

My understanding was the it was the first paragraph, and not sentence. Fixed.

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/interna...
pkg/path/lib/src/internal_style.dart:50: /// Gets if [path] is root-relative.
On 2014/08/05 20:22:26, nweiz wrote:
> "Gets if" -> "Returns whether"

Done.

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/style/u...
File pkg/path/lib/src/style/url.dart (right):

https://codereview.chromium.org/439223002/diff/20001/pkg/path/lib/src/style/u...
pkg/path/lib/src/style/url.dart:39: return rootLength(path) > 0 &&
path.endsWith("://");
On 2014/08/05 20:22:27, nweiz wrote:
> I believe this still breaks on "http://foo.com/bar://". I suggest:
> 
>     return rootLength(path) == path.length && path.endsWith("://");

Better. Fixed. Also, swapped them as endsWith is faster than rootLength.

[nweiz, 2014-08-06 21:05:48]

lgtm

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
File pkg/path/lib/src/style/windows.dart (right):

https://codereview.chromium.org/439223002/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:47: return path.length;
On 2014/08/06 07:05:15, Anders Johnsen wrote:
> On 2014/08/05 20:22:26, nweiz wrote:
> > On 2014/08/05 08:57:14, Anders Johnsen wrote:
> > > On 2014/08/04 20:11:58, nweiz wrote:
> > > > I believe Windows considers "\\foo" and "\\foo\bar" to be equivalent to
> > "\foo"
> > > > and "\foo\bar" respectively, rather than UNC paths.
> > > 
> > > Surprisingly, this is not tested. Can you find some documentation behind
> this,
> > > so I can change it accordingly?
> > 
> > http://msdn.microsoft.com/en-us/library/gg465305.aspx
> > 
> > Looks like "\\foo" isn't a valid UNC path, but "\\foo\bar" is. We should
> > definitely add tests for these.
> 
> But saying it's invalid doesn't indicate how we then should handle it in this
> case. If I say '\\localhost' in start->run, it'll take me to a list of network
> shares on my computer. I think we'll just have to treat it as the root in any
> case. It's not, by any mean, a relative root. Thoughts?

SGTM

[Anders Johnsen, 2014-08-07 08:11:43]

Committed patchset #4 manually as 38966 (presubmit successful).