path: replace duplicate conditions by functions #18693

daynin · 2018-02-10T11:08:14Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

path

A benchmark results (there are no significant changes):

                                                                                                   confidence improvement accuracy (*)   (**)  (***)
 path/basename-posix.js n=1000000 pathext=""                                                                        0.57 %       ±0.86% ±1.15% ±1.49%
 path/basename-posix.js n=1000000 pathext="/"                                                                      -0.43 %       ±0.86% ±1.15% ±1.50%
 path/basename-posix.js n=1000000 pathext="/foo"                                                                    0.26 %       ±0.67% ±0.89% ±1.16%
 path/basename-posix.js n=1000000 pathext="/foo/.bar.baz"                                                          -0.09 %       ±0.28% ±0.37% ±0.48%
 path/basename-posix.js n=1000000 pathext="/foo/.bar.baz|.baz"                                                     -0.08 %       ±0.17% ±0.23% ±0.29%
 path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html"                                             0.05 %       ±0.21% ±0.27% ±0.36%
 path/basename-posix.js n=1000000 pathext="/foo/bar/baz/asdf/quux.html|.html"                                      -0.05 %       ±0.16% ±0.21% ±0.27%
 path/basename-posix.js n=1000000 pathext="foo"                                                                     0.10 %       ±0.48% ±0.64% ±0.83%
 path/basename-posix.js n=1000000 pathext="foo/bar."                                                                0.00 %       ±0.30% ±0.40% ±0.52%
 path/basename-posix.js n=1000000 pathext="foo/bar.|."                                                              0.01 %       ±0.28% ±0.37% ±0.49%
 path/basename-win32.js n=1000000 pathext=""                                                                        0.09 %       ±0.75% ±1.00% ±1.30%
 path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html"                             -0.12 %       ±0.20% ±0.26% ±0.34%
 path/basename-win32.js n=1000000 pathext="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.html|.html"                        0.03 %       ±0.13% ±0.18% ±0.23%
 path/basename-win32.js n=1000000 pathext="C:\\\\"                                                                  0.03 %       ±0.42% ±0.56% ±0.73%
 path/basename-win32.js n=1000000 pathext="C:\\\\foo"                                                              -0.03 %       ±0.40% ±0.53% ±0.70%
 path/basename-win32.js n=1000000 pathext="D:\\\\foo\\\\.bar.baz"                                                  -0.06 %       ±0.19% ±0.26% ±0.33%
 path/basename-win32.js n=1000000 pathext="E:\\\\foo\\\\.bar.baz|.baz"                                       *      0.17 %       ±0.15% ±0.20% ±0.26%
 path/basename-win32.js n=1000000 pathext="foo"                                                                    -0.26 %       ±0.63% ±0.84% ±1.11%
 path/basename-win32.js n=1000000 pathext="foo\\\\bar."                                                            -0.22 %       ±0.24% ±0.32% ±0.42%
 path/basename-win32.js n=1000000 pathext="foo\\\\bar.|."                                                          -0.06 %       ±0.23% ±0.31% ±0.40%
 path/dirname-posix.js n=1000000 path=""                                                                            0.73 %       ±1.60% ±2.14% ±2.79%
 path/dirname-posix.js n=1000000 path="/"                                                                          -0.18 %       ±0.88% ±1.17% ±1.53%
 path/dirname-posix.js n=1000000 path="/foo"                                                                        0.23 %       ±0.61% ±0.81% ±1.05%
 path/dirname-posix.js n=1000000 path="/foo/bar"                                                                    0.25 %       ±0.73% ±0.97% ±1.26%
 path/dirname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux"                                                     -0.13 %       ±0.35% ±0.47% ±0.61%
 path/dirname-posix.js n=1000000 path="foo"                                                                        -0.10 %       ±0.85% ±1.14% ±1.48%
 path/dirname-posix.js n=1000000 path="foo/bar"                                                                    -0.13 %       ±0.80% ±1.06% ±1.39%
 path/dirname-win32.js n=1000000 path=""                                                                            0.03 %       ±0.82% ±1.09% ±1.41%
 path/dirname-win32.js n=1000000 path="\\\\"                                                                        0.37 %       ±0.70% ±0.93% ±1.21%
 path/dirname-win32.js n=1000000 path="\\\\foo"                                                                    -0.39 %       ±0.51% ±0.68% ±0.89%
 path/dirname-win32.js n=1000000 path="C:\\\\foo\\\\bar"                                                           -0.33 %       ±0.50% ±0.67% ±0.88%
 path/dirname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                                    -0.05 %       ±0.22% ±0.29% ±0.38%
 path/dirname-win32.js n=1000000 path="foo"                                                                        -0.47 %       ±0.49% ±0.65% ±0.85%
 path/dirname-win32.js n=1000000 path="foo\\\\bar"                                                                  0.08 %       ±0.34% ±0.46% ±0.60%
 path/extname-posix.js n=1000000 path=""                                                                            0.05 %       ±0.79% ±1.05% ±1.37%
 path/extname-posix.js n=1000000 path="/"                                                                           0.10 %       ±0.75% ±1.00% ±1.30%
 path/extname-posix.js n=1000000 path="/foo"                                                                       -0.11 %       ±0.32% ±0.43% ±0.56%
 path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux.foobarbazasdfquux"                                   -0.05 %       ±0.11% ±0.14% ±0.19%
 path/extname-posix.js n=1000000 path="/foo/bar/baz/asdf/quux"                                                     -0.07 %       ±0.37% ±0.49% ±0.64%
 path/extname-posix.js n=1000000 path="foo/.bar.baz"                                                               -0.16 %       ±0.21% ±0.27% ±0.36%
 path/extname-posix.js n=1000000 path="foo/bar/...baz.quux"                                                         0.09 %       ±0.16% ±0.22% ±0.29%
 path/extname-posix.js n=1000000 path="foo/bar/..baz.quux"                                                          0.11 %       ±0.13% ±0.17% ±0.22%
 path/extname-posix.js n=1000000 path="index.html"                                                                 -0.25 %       ±0.52% ±0.70% ±0.92%
 path/extname-posix.js n=1000000 path="index"                                                                       0.11 %       ±0.24% ±0.32% ±0.41%
 path/extname-win32.js n=1000000 path=""                                                                           -0.50 %       ±1.06% ±1.41% ±1.84%
 path/extname-win32.js n=1000000 path="\\\\"                                                                 *     -1.08 %       ±0.83% ±1.10% ±1.44%
 path/extname-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux.foobarbazasdfquux"                    -0.04 %       ±0.22% ±0.29% ±0.38%
 path/extname-win32.js n=1000000 path="C:\\\\foo"                                                                  -0.33 %       ±0.41% ±0.54% ±0.71%
 path/extname-win32.js n=1000000 path="D:\\\\foo\\\\bar\\\\baz\\\\asdf\\\\quux"                                    -0.20 %       ±0.81% ±1.08% ±1.41%
 path/extname-win32.js n=1000000 path="foo\\\\.bar.baz"                                                             0.08 %       ±0.30% ±0.39% ±0.51%
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\...baz.quux"                                            *     -0.21 %       ±0.20% ±0.27% ±0.35%
 path/extname-win32.js n=1000000 path="foo\\\\bar\\\\..baz.quux"                                                    0.03 %       ±0.34% ±0.45% ±0.58%
 path/extname-win32.js n=1000000 path="index.html"                                                           *      0.27 %       ±0.22% ±0.30% ±0.39%
 path/extname-win32.js n=1000000 path="index"                                                                       0.20 %       ±0.56% ±0.75% ±0.97%
 path/format-posix.js n=10000000 props="/|/home/user/dir|index.html|.html|index"                                    0.18 %       ±0.28% ±0.38% ±0.49%
 path/format-win32.js n=10000000 props="C:\\\\|C:\\\\path\\\\dir|index.html|.html|index"                           -0.02 %       ±0.28% ±0.37% ±0.48%
 path/isAbsolute-posix.js n=1000000 path="."                                                                       -0.41 %       ±1.15% ±1.53% ±2.00%
 path/isAbsolute-posix.js n=1000000 path=""                                                                         0.18 %       ±1.21% ±1.62% ±2.11%
 path/isAbsolute-posix.js n=1000000 path="/baz/.."                                                                 -0.95 %       ±1.18% ±1.57% ±2.05%
 path/isAbsolute-posix.js n=1000000 path="/foo/bar"                                                                -0.74 %       ±1.01% ±1.34% ±1.75%
 path/isAbsolute-posix.js n=1000000 path="bar/baz"                                                                  0.30 %       ±1.27% ±1.69% ±2.20%
 path/isAbsolute-win32.js n=1000000 path="."                                                                       -0.25 %       ±0.94% ±1.25% ±1.63%
 path/isAbsolute-win32.js n=1000000 path=""                                                                         0.42 %       ±1.35% ±1.79% ±2.34%
 path/isAbsolute-win32.js n=1000000 path="//server"                                                                 0.00 %       ±1.23% ±1.64% ±2.13%
 path/isAbsolute-win32.js n=1000000 path="bar\\\\baz"                                                              -0.06 %       ±0.63% ±0.84% ±1.10%
 path/isAbsolute-win32.js n=1000000 path="C:\\\\baz\\\\.."                                                         -0.37 %       ±0.58% ±0.78% ±1.02%
 path/isAbsolute-win32.js n=1000000 path="C:baz\\\\.."                                                              0.24 %       ±0.78% ±1.04% ±1.37%
 path/join-posix.js n=1000000 paths="/foo|bar||baz/asdf|quux|.."                                                    0.10 %       ±0.15% ±0.20% ±0.26%
 path/join-win32.js n=1000000 paths="C:\\\\foo|bar||baz\\\\asdf|quux|.."                                           -0.13 %       ±0.18% ±0.23% ±0.31%
 path/makeLong-win32.js n=1000000 path="\\\\\\\\?\\\\foo"                                                          -0.15 %       ±0.57% ±0.76% ±0.99%
 path/makeLong-win32.js n=1000000 path="\\\\\\\\foo\\\\bar"                                                        -0.21 %       ±0.58% ±0.78% ±1.01%
 path/makeLong-win32.js n=1000000 path="C:\\\\foo"                                                                 -0.24 %       ±0.48% ±0.64% ±0.84%
 path/makeLong-win32.js n=1000000 path="foo\\\\bar"                                                                -0.32 %       ±1.36% ±1.82% ±2.37%
 path/normalize-posix.js n=1000000 path="."                                                                         0.27 %       ±0.43% ±0.58% ±0.76%
 path/normalize-posix.js n=1000000 path=""                                                                         -0.08 %       ±0.67% ±0.90% ±1.17%
 path/normalize-posix.js n=1000000 path="/../"                                                                     -0.24 %       ±0.67% ±0.89% ±1.17%
 path/normalize-posix.js n=1000000 path="/foo"                                                                      0.07 %       ±0.46% ±0.61% ±0.79%
 path/normalize-posix.js n=1000000 path="/foo/bar"                                                                 -0.07 %       ±0.28% ±0.37% ±0.48%
 path/normalize-posix.js n=1000000 path="/foo/bar//baz/asdf/quux/.."                                                0.27 %       ±0.52% ±0.69% ±0.92%
 path/normalize-win32.js n=1000000 path="."                                                                         0.25 %       ±0.35% ±0.47% ±0.61%
 path/normalize-win32.js n=1000000 path=""                                                                          0.65 %       ±0.77% ±1.03% ±1.34%
 path/normalize-win32.js n=1000000 path="C:\\\\..\\\\"                                                              0.06 %       ±0.40% ±0.54% ±0.70%
 path/normalize-win32.js n=1000000 path="C:\\\\foo"                                                                 0.09 %       ±0.44% ±0.58% ±0.76%
 path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar"                                                         -0.26 %       ±0.44% ±0.59% ±0.76%
 path/normalize-win32.js n=1000000 path="C:\\\\foo\\\\bar\\\\\\\\baz\\\\asdf\\\\quux\\\\.."                         0.08 %       ±0.25% ±0.34% ±0.44%
 path/parse-posix.js n=1000000 path=""                                                                       *     -0.99 %       ±0.96% ±1.28% ±1.67%
 path/parse-posix.js n=1000000 path="/"                                                                             1.01 %       ±3.71% ±4.94% ±6.43%
 path/parse-posix.js n=1000000 path="/foo"                                                                         -0.01 %       ±0.39% ±0.52% ±0.67%
 path/parse-posix.js n=1000000 path="/foo/bar.baz"                                                                  0.02 %       ±0.38% ±0.51% ±0.66%
 path/parse-posix.js n=1000000 path="/foo/bar/baz/asdf/.quux"                                                      -0.00 %       ±0.19% ±0.25% ±0.33%
 path/parse-posix.js n=1000000 path="foo/.bar.baz"                                                                 -0.12 %       ±0.37% ±0.49% ±0.64%
 path/parse-posix.js n=1000000 path="foo/bar"                                                                       0.14 %       ±0.39% ±0.53% ±0.68%
 path/parse-win32.js n=1000000 path=""                                                                             -0.39 %       ±0.82% ±1.09% ±1.41%
 path/parse-win32.js n=1000000 path="\\\\foo"                                                                       0.10 %       ±0.42% ±0.56% ±0.73%
 path/parse-win32.js n=1000000 path="\\\\foo\\\\bar\\\\baz\\\\asdf\\\\.quux"                                       -0.09 %       ±0.21% ±0.28% ±0.37%
 path/parse-win32.js n=1000000 path="C:\\\\"                                                                        0.40 %       ±0.81% ±1.07% ±1.40%
 path/parse-win32.js n=1000000 path="C:\\\\foo"                                                                     0.11 %       ±0.64% ±0.85% ±1.10%
 path/parse-win32.js n=1000000 path="E:\\\\foo\\\\bar.baz"                                                         -0.13 %       ±0.17% ±0.23% ±0.30%
 path/parse-win32.js n=1000000 path="foo\\\\.bar.baz"                                                               0.21 %       ±0.33% ±0.45% ±0.58%
 path/parse-win32.js n=1000000 path="foo\\\\bar"                                                                   -0.03 %       ±0.45% ±0.59% ±0.78%
 path/relative-posix.js n=1000000 paths="/|/"                                                                      -0.45 %       ±0.95% ±1.26% ±1.64%
 path/relative-posix.js n=1000000 paths="/|/var"                                                                   -0.04 %       ±0.30% ±0.40% ±0.52%
 path/relative-posix.js n=1000000 paths="/data/orandea/test/aaa|/data/orandea/impl/bbb"                             0.14 %       ±0.24% ±0.32% ±0.42%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/"                                                      -0.07 %       ±0.10% ±0.13% ±0.17%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/foo/bar/baz/quux"                                       0.52 %       ±0.98% ±1.30% ±1.69%
 path/relative-posix.js n=1000000 paths="/foo/bar/baz/quux|/var/log"                                                0.05 %       ±0.29% ±0.38% ±0.50%
 path/relative-posix.js n=1000000 paths="/var|/bin"                                                                 0.01 %       ±0.40% ±0.53% ±0.69%
 path/relative-win32.js n=1000000 paths="C:\\\\|D:\\\\"                                                            -0.09 %       ±0.18% ±0.24% ±0.32%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz\\\\quux|C:\\\\"                                    0.15 %       ±0.26% ±0.34% ±0.45%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\bar\\\\baz|C:\\\\foo\\\\bar\\\\baz"                          -0.17 %       ±0.39% ±0.51% ±0.67%
 path/relative-win32.js n=1000000 paths="C:\\\\foo\\\\BAR\\\\BAZ|C:\\\\foo\\\\bar\\\\baz"                           0.13 %       ±0.59% ±0.79% ±1.03%
 path/relative-win32.js n=1000000 paths="C:\\\\orandea\\\\test\\\\aaa|C:\\\\orandea\\\\impl\\\\bbb"                -1.00 %       ±1.85% ±2.49% ±3.30%
 path/resolve-posix.js n=1000000 paths=""                                                                           0.19 %       ±1.66% ±2.22% ±2.88%
 path/resolve-posix.js n=1000000 paths="|"                                                                         -0.17 %       ±2.09% ±2.79% ±3.66%
 path/resolve-posix.js n=1000000 paths="a/b/c/|../../.."                                                           -0.63 %       ±1.50% ±2.00% ±2.60%
 path/resolve-posix.js n=1000000 paths="foo/bar|/tmp/file/|..|a/../subfile"                                        -0.20 %       ±0.86% ±1.15% ±1.50%
 path/resolve-win32.js n=1000000 paths=""                                                                           0.20 %       ±1.53% ±2.04% ±2.65%
 path/resolve-win32.js n=1000000 paths="|"                                                                          0.54 %       ±1.35% ±1.80% ±2.34%
 path/resolve-win32.js n=1000000 paths="c:/blah\\\\blah|d:/games|c:../a"                                           -0.01 %       ±0.41% ±0.55% ±0.71%
 path/resolve-win32.js n=1000000 paths="c:/ignore|d:\\\\a/b\\\\c/d|\\\\e.exe"                                      -0.07 %       ±0.59% ±0.79% ±1.03%

bnoordhuis

Nice, LGTM. The change in performance doesn't look statistically significant.

CI: https://ci.nodejs.org/job/node-test-pull-request/13076/

daynin · 2018-02-12T22:34:47Z

I resolved a conflict here

apapirovski

This is nice! Just some nits below.

apapirovski · 2018-02-12T22:36:46Z

lib/path.js

+
+function isWindowsDeviceRoot(code) {
+  return code >= CHAR_UPPERCASE_A && code <= CHAR_UPPERCASE_Z ||
+    code >= CHAR_LOWERCASE_A && code <= CHAR_LOWERCASE_Z;


Nit: this would be slightly nicer aligned with the code part above, rather than just indented two spaces.

apapirovski · 2018-02-12T22:37:31Z

lib/path.js

      // `path` contains just a path separator, exit early to avoid unnecessary
      // work
      return '\\';
    }

    code = path.charCodeAt(len - 1);
-    var trailingSeparator =
-      (code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH);
+    var trailingSeparator = (isPathSeparator(code));


Unnecessary parentheses here.

I would also inline path.charCodeAt(len - 1).

apapirovski · 2018-02-12T22:42:09Z

lib/path.js

@@ -935,15 +935,14 @@ const win32 = {
    if (path.length >= 2) {
      const code = path.charCodeAt(0);


This is unnecessary. This could all be just one if statement. path.length >= 2 && path.charCodeAt(1) === CHAR_COLON && isWindowsDeviceRoot(path.charCodeAt(0))

BridgeAR

LGTM. I would just go ahead and inline lots of charCodeAt calls.

BridgeAR · 2018-02-12T23:01:39Z

lib/path.js

      // `path` contains just a path separator, exit early to avoid unnecessary
      // work
      return '\\';
    }

    code = path.charCodeAt(len - 1);
-    var trailingSeparator =
-      (code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH);
+    var trailingSeparator = (isPathSeparator(code));


I would also inline path.charCodeAt(len - 1).

BridgeAR · 2018-02-12T23:02:20Z

lib/path.js

-                    code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH;
-
-                  if (isPathSeparator)
+                  if (isPathSeparator(code))


It should be fine to inline path.charCodeAt(j).

BridgeAR · 2018-02-12T23:02:42Z

lib/path.js

          // Possible UNC root

          // If we started with a separator, we know we at least have an
          // absolute path of some kind (UNC or otherwise)
          isAbsolute = true;

          code = path.charCodeAt(1);
-          if (code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH) {
+          if (isPathSeparator(code)) {


It should be fine to inline path.charCodeAt(1).

BridgeAR · 2018-02-12T23:02:56Z

lib/path.js

            // Matched double path separator at beginning
            var j = 2;
            var last = j;
            // Match 1 or more non-path separators
            for (; j < len; ++j) {
              code = path.charCodeAt(j);
-              if (code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH)
+              if (isPathSeparator(code))


It should be fine to inline path.charCodeAt(j)

BridgeAR · 2018-02-12T23:03:13Z

lib/path.js

@@ -259,7 +269,7 @@ const win32 = {
              // Match 1 or more path separators
              for (; j < len; ++j) {
                code = path.charCodeAt(j);
-                if (code !== CHAR_FORWARD_SLASH && code !== CHAR_BACKWARD_SLASH)
+                if (!isPathSeparator(code))


It should be fine to inline pathCharCodeAt(j)

BridgeAR · 2018-02-12T23:05:29Z

lib/path.js

          // Possible device root

          if (path.charCodeAt(1) === CHAR_COLON) {
            device = path.slice(0, 2);
            rootEnd = 2;
            if (len > 2) {
              code = path.charCodeAt(2);
-              if (code === CHAR_FORWARD_SLASH || code === CHAR_BACKWARD_SLASH) {
+              if (isPathSeparator(code)) {


It should be fine to inline path.charCodeAt(2)

Ok, this applies to a lot of places here ;-)

@BridgeAR there are too many places like this. I think it will be better to fix it in a another pull request. What do you think?

Hm, I personally think this would actually be the right place to do so. The reason is that it touches the same lines. @apapirovski what do you think?

code seems to mostly be there to avoid constantly calling path.charCodeAt(x) within the same conditional but now that these are a function, it seems like their purpose is gone. I would say that this could definitely be done in this PR even though it adds a bunch of work.

@BridgeAR ok, I was just worried that it might affect some logic here. Nonetheless, I'll fix it

@daynin it'll probably take some effort to validate all the places where it's safe to do so. Would suggest doing it as a separate commit. Should make reviews easier.

@apaprocki ok, sure

It should be relatively straight forward. Try to remove them from the most inner function and go to the outer one. I checked it for one big function and as far as I saw it was possible to remove all of them besides a single one that should switch to be const.

@apapirovski @BridgeAR fixed

apapirovski · 2018-02-13T16:47:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13148/

BridgeAR · 2018-02-13T16:56:19Z

lib/path.js

      return true;
-    } else if ((code >= CHAR_UPPERCASE_A && code <= CHAR_UPPERCASE_Z) ||
-               (code >= CHAR_LOWERCASE_A && code <= CHAR_LOWERCASE_Z)) {
+    } else if (isWindowsDeviceRoot(path.charCodeAt(0))) {


Here in this case it would be better to keep the code above as it will otherwise require two lookups for this branch.

BridgeAR · 2018-02-13T16:59:04Z

lib/path.js

@@ -958,7 +929,7 @@ const win32 = {
        matchedSlash = false;
        end = i + 1;
      }
-      if (code === CHAR_DOT) {
+      if (path.charCodeAt(i) === CHAR_DOT) {


Here it would be better to keep code because this branch otherwise has to do a second lookup.

BridgeAR · 2018-02-13T17:10:58Z

lib/path.js

@@ -1571,7 +1529,7 @@ const posix = {
        matchedSlash = false;
        end = i + 1;
      }
-      if (code === CHAR_DOT) {
+      if (path.charCodeAt(i) === CHAR_DOT) {


Here it would be better to keep the code because of a second lookup.

BridgeAR · 2018-02-13T17:17:14Z

lib/path.js

@@ -1492,7 +1452,7 @@ const posix = {
        matchedSlash = false;
        end = i + 1;
      }
-      if (code === CHAR_DOT) {
+      if (path.charCodeAt(i) === CHAR_DOT) {


Please code here to prevent a second lookup.

BridgeAR · 2018-02-13T17:17:22Z

lib/path.js

@@ -1421,7 +1382,7 @@ const posix = {
          }
          if (extIdx >= 0) {
            // Try to match the explicit extension
-            if (code === ext.charCodeAt(extIdx)) {
+            if (path.charCodeAt(i) === ext.charCodeAt(extIdx)) {


Please keep code here.

BridgeAR · 2018-02-13T17:18:51Z

lib/path.js

@@ -1115,7 +1079,7 @@ const win32 = {
        matchedSlash = false;
        end = i + 1;
      }
-      if (code === CHAR_DOT) {
+      if (path.charCodeAt(i) === CHAR_DOT) {


Please keep code here.

daynin · 2018-02-14T09:54:11Z

@BridgeAR fixed

BridgeAR · 2018-02-16T18:08:09Z

CI https://ci.nodejs.org/job/node-test-pull-request/13216/

BridgeAR · 2018-02-17T13:54:22Z

Landed in b404aa5 🎉

It will also remove useless "code" variables by inlining path.charCodeAt. PR-URL: nodejs#18693 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

It will also remove useless "code" variables by inlining path.charCodeAt. PR-URL: #18693 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

It will also remove useless "code" variables by inlining path.charCodeAt. PR-URL: nodejs#18693 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Feb 10, 2018

bnoordhuis approved these changes Feb 10, 2018

View reviewed changes

tniessen approved these changes Feb 10, 2018

View reviewed changes

BridgeAR approved these changes Feb 10, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 10, 2018

cjihrig approved these changes Feb 10, 2018

View reviewed changes

lpinca approved these changes Feb 11, 2018

View reviewed changes

maclover7 approved these changes Feb 12, 2018

View reviewed changes

daynin force-pushed the path-refactor-conditions branch from 32810c4 to 173c4f7 Compare February 12, 2018 22:34

daynin force-pushed the path-refactor-conditions branch from 173c4f7 to 6120bf5 Compare February 12, 2018 22:38

apapirovski approved these changes Feb 12, 2018

View reviewed changes

BridgeAR approved these changes Feb 12, 2018

View reviewed changes

path: replace duplicate conditions by functions

6853af6

daynin force-pushed the path-refactor-conditions branch from 6120bf5 to 6853af6 Compare February 12, 2018 23:15

BridgeAR reviewed Feb 13, 2018

View reviewed changes

jasnell approved these changes Feb 13, 2018

View reviewed changes

daynin force-pushed the path-refactor-conditions branch from a33bf9c to b18a1a3 Compare February 14, 2018 09:40

path: remove useless "code" variable, inline path.charCodeAt

1825066

daynin force-pushed the path-refactor-conditions branch from b18a1a3 to 1825066 Compare February 14, 2018 09:46

BridgeAR approved these changes Feb 16, 2018

View reviewed changes

BridgeAR closed this Feb 17, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the dont-land-on-v6.x label Apr 13, 2018

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

path: replace duplicate conditions by functions #18693

path: replace duplicate conditions by functions #18693

daynin commented Feb 10, 2018 •

edited

Loading

bnoordhuis left a comment

daynin commented Feb 12, 2018

apapirovski left a comment

apapirovski Feb 12, 2018

apapirovski Feb 12, 2018

BridgeAR Feb 12, 2018

apapirovski Feb 12, 2018

BridgeAR left a comment

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

BridgeAR Feb 12, 2018

daynin Feb 12, 2018 •

edited

Loading

BridgeAR Feb 12, 2018

apapirovski Feb 12, 2018

daynin Feb 12, 2018

apapirovski Feb 12, 2018

daynin Feb 12, 2018

BridgeAR Feb 12, 2018 •

edited

Loading

daynin Feb 13, 2018

apapirovski commented Feb 13, 2018

BridgeAR Feb 13, 2018

BridgeAR Feb 13, 2018

BridgeAR Feb 13, 2018

BridgeAR Feb 13, 2018

BridgeAR Feb 13, 2018

BridgeAR Feb 13, 2018

daynin commented Feb 14, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Feb 17, 2018

		@@ -935,15 +935,14 @@ const win32 = {
		if (path.length >= 2) {
		const code = path.charCodeAt(0);

path: replace duplicate conditions by functions #18693

path: replace duplicate conditions by functions #18693

Conversation

daynin commented Feb 10, 2018 • edited Loading

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

daynin commented Feb 12, 2018

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daynin Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daynin commented Feb 14, 2018

BridgeAR commented Feb 16, 2018

BridgeAR commented Feb 17, 2018

daynin commented Feb 10, 2018 •

edited

Loading

daynin Feb 12, 2018 •

edited

Loading

BridgeAR Feb 12, 2018 •

edited

Loading