Issue 309803003: Avoid using RegExps in path to work around issue 19090.

Issue 309803003: Avoid using RegExps in path to work around issue 19090. (Closed)

Created:
6 years, 6 months ago by nweiz

Modified:
6 years, 6 months ago

Reviewers:
Bob Nystrom

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Avoid using RegExps in path to work around issue 19090. R=rnystrom@google.com Committed: https://code.google.com/p/dart/source/detail?r=36909

Patch Set 2 : code review #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+215 lines, -50 lines)			Patch
M	pkg/path/CHANGELOG.md	View		1 chunk	+3 lines, -0 lines	0 comments	Download
A	pkg/path/lib/src/characters.dart	View	1	1 chunk	+19 lines, -0 lines	0 comments	Download
M	pkg/path/lib/src/context.dart	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	pkg/path/lib/src/internal_style.dart	View	1	1 chunk	+12 lines, -33 lines	0 comments	Download
M	pkg/path/lib/src/parsed_path.dart	View	1	2 chunks	+12 lines, -11 lines	0 comments	Download
M	pkg/path/lib/src/style/posix.dart	View	1	2 chunks	+20 lines, -0 lines	0 comments	Download
M	pkg/path/lib/src/style/url.dart	View	1	2 chunks	+63 lines, -0 lines	0 comments	Download
M	pkg/path/lib/src/style/windows.dart	View	1	3 chunks	+66 lines, -2 lines	0 comments	Download
A	pkg/path/lib/src/utils.dart	View	1	1 chunk	+16 lines, -0 lines	0 comments	Download
M	pkg/path/pubspec.yaml	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Bob Nystrom

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dart File pkg/path/lib/src/context.dart (right): https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dart#newcode233 pkg/path/lib/src/context.dart:233: if (part.length > 0 && style.separators.any(part[0].contains)) { How about ...

6 years, 6 months ago (2014-06-02 22:43:05 UTC) #2

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:233: if (part.length > 0 &&
style.separators.any(part[0].contains)) {
How about making this a method too:

style.containsSeparator(part[0])

This would let us avoid creating a closure and looping on POSIX and URL. On my
local tests, it's a bit faster.

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/parsed_path...
File pkg/path/lib/src/parsed_path.dart (right):

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/parsed_path...
pkg/path/lib/src/parsed_path.dart:55: var firstSeparator =
style.separators.firstWhere(path.startsWith,
I got a noticeable speed bump (1.61s -> 1.37s) by making this functional too.
Change this whole block to something like:

style.splitPath(path, separators, parts);

Then implement that in the POSIX/URL styles like:

  void splitPath(path, separators, parts) {
    var start = 0;

    if (path.isNotEmpty && path.codeUnitAt(0) == 0x2f /* '/' */) {
      separators.add('/');
      start = 1;
    } else {
      separators.add('');
    }

    for (var i = start; i < path.length; i++) {
      if (path.codeUnitAt(i) == 0x2f /* '/' */) {
        parts.add(path.substring(start, i));
        separators.add(separator);
        start = i + 1;
      }
    }

    // Add the final part, if any.
    if (start < path.length) {
      parts.add(path.substring(start));
      separators.add('');
    }
  }

And Windows:


  void splitPath(path, separators, parts) {
    var start = 0;

    if (path.isNotEmpty &&
        (path.codeUnitAt(0) == 0x2f /* '/' */ || path.codeUnitAt(0) == 0x5c /*
'\' */)) {
      separators.add(path[0]);
      start = 1;
    } else {
      separators.add('');
    }

    for (var i = start; i < path.length; i++) {
      if (path.codeUnitAt(i) == 0x2f /* '/' */ || path.codeUnitAt(i) == 0x5c /*
'\' */) {
        parts.add(path.substring(start, i));
        separators.add(path[i]);
        start = i + 1;
      }
    }

    // Add the final part, if any.
    if (start < path.length) {
      parts.add(path.substring(start));
      separators.add('');
    }
  }

This would let you ditch .separators on Style, which gets it closer to a pure
method-based API.

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

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/style/posix...
pkg/path/lib/src/style/posix.dart:26: !path.isEmpty &&
path.codeUnitAt(path.length - 1) != 0x2f; // "/"
!path.isEmpty -> path.isNotEmpty

and elsewhere.

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

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:107: if (path.codeUnitAt(1) != 0x3a) return
null; // ":"
":".

Or, better, make a little class with constants for these chars, like:

class Chars {
  static const COLON = 0x3a;
  ...
}

?

nweiz

6 years, 6 months ago (2014-06-02 23:13:24 UTC) #3

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:233: if (part.length > 0 &&
style.separators.any(part[0].contains)) {
On 2014/06/02 22:43:05, Bob Nystrom wrote:
> How about making this a method too:
> 
> style.containsSeparator(part[0])
> 
> This would let us avoid creating a closure and looping on POSIX and URL. On my
> local tests, it's a bit faster.

Done.

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/parsed_path...
File pkg/path/lib/src/parsed_path.dart (right):

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/parsed_path...
pkg/path/lib/src/parsed_path.dart:55: var firstSeparator =
style.separators.firstWhere(path.startsWith,
On 2014/06/02 22:43:05, Bob Nystrom wrote:
> I got a noticeable speed bump (1.61s -> 1.37s) by making this functional too.
> Change this whole block to something like:
> 
> style.splitPath(path, separators, parts);
> 
> Then implement that in the POSIX/URL styles like:
> 
>   void splitPath(path, separators, parts) {
>     var start = 0;
> 
>     if (path.isNotEmpty && path.codeUnitAt(0) == 0x2f /* '/' */) {
>       separators.add('/');
>       start = 1;
>     } else {
>       separators.add('');
>     }
> 
>     for (var i = start; i < path.length; i++) {
>       if (path.codeUnitAt(i) == 0x2f /* '/' */) {
>         parts.add(path.substring(start, i));
>         separators.add(separator);
>         start = i + 1;
>       }
>     }
> 
>     // Add the final part, if any.
>     if (start < path.length) {
>       parts.add(path.substring(start));
>       separators.add('');
>     }
>   }
> 
> And Windows:
> 
> 
>   void splitPath(path, separators, parts) {
>     var start = 0;
> 
>     if (path.isNotEmpty &&
>         (path.codeUnitAt(0) == 0x2f /* '/' */ || path.codeUnitAt(0) == 0x5c /*
> '\' */)) {
>       separators.add(path[0]);
>       start = 1;
>     } else {
>       separators.add('');
>     }
> 
>     for (var i = start; i < path.length; i++) {
>       if (path.codeUnitAt(i) == 0x2f /* '/' */ || path.codeUnitAt(i) == 0x5c
/*
> '\' */) {
>         parts.add(path.substring(start, i));
>         separators.add(path[i]);
>         start = i + 1;
>       }
>     }
> 
>     // Add the final part, if any.
>     if (start < path.length) {
>       parts.add(path.substring(start));
>       separators.add('');
>     }
>   }
> 
> This would let you ditch .separators on Style, which gets it closer to a pure
> method-based API.

Done, with a little less code duplication.

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

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/style/posix...
pkg/path/lib/src/style/posix.dart:26: !path.isEmpty &&
path.codeUnitAt(path.length - 1) != 0x2f; // "/"
On 2014/06/02 22:43:05, Bob Nystrom wrote:
> !path.isEmpty -> path.isNotEmpty
> 
> and elsewhere.

Done.

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

https://codereview.chromium.org/309803003/diff/1/pkg/path/lib/src/style/windo...
pkg/path/lib/src/style/windows.dart:107: if (path.codeUnitAt(1) != 0x3a) return
null; // ":"
On 2014/06/02 22:43:05, Bob Nystrom wrote:
> ":".
> 
> Or, better, make a little class with constants for these chars, like:
> 
> class Chars {
>   static const COLON = 0x3a;
>   ...
> }
> 
> ?

Done.

nweiz

6 years, 6 months ago (2014-06-02 23:57:15 UTC) #5

Message was sent while issue was closed.

Committed patchset #2 manually as r36909 (presubmit successful).

Expand Messages | Collapse Messages