FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

path: replace duplicate conditions by functions by daynin · Pull Request #18693 · nodejs/node · GitHub

/ node Public

path: replace duplicate conditions by functions - #18693

Closed
daynin wants to merge 2 commits into
nodejs:masterfrom
daynin:path-refactor-conditions
ConversationCommits (2)ChecksFiles changed
Closed

path: replace duplicate conditions by functions#18693
daynin wants to merge 2 commits into
nodejs:masterfrom
daynin:path-refactor-conditions

Conversation

daynin commented Feb 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor
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%

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

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 10, 2018
daynin force-pushed the path-refactor-conditions branch from 32810c4 to 173c4f7 Compare February 12, 2018 22:34

daynin commented Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

I resolved a conflict here

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

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is nice! Just some nits below.

Comment thread lib/path.js Outdated

apapirovski Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

apapirovski Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unnecessary parentheses here.

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

apapirovski Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR approved these changes Feb 12, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It should be fine to inline pathCharCodeAt(j)

Comment thread lib/path.js Outdated

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

daynin Feb 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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?

BridgeAR Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

apapirovski Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

daynin Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

apapirovski Feb 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

daynin Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@apaprocki ok, sure

BridgeAR Feb 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

daynin Feb 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@apapirovski @BridgeAR fixed

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

apapirovski commented Feb 13, 2018

Copy link
Copy Markdown
Contributor

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

BridgeAR reviewed Feb 13, 2018
View reviewed changes
Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please code here to prevent a second lookup.

Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please keep code here.

Comment thread lib/path.js Outdated

BridgeAR Feb 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please keep code here.

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

daynin commented Feb 14, 2018

Copy link
Copy Markdown
Contributor Author

@BridgeAR fixed

BridgeAR commented Feb 16, 2018

Copy link
Copy Markdown
Member

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

BridgeAR commented Feb 17, 2018

Copy link
Copy Markdown
Member

Landed in b404aa5 🎉

BridgeAR closed this Feb 17, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 17, 2018
path: replace duplicate conditions by functions
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
path: replace duplicate conditions by functions
be08ca7 
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>
MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
path: replace duplicate conditions by functions
80c6142 
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
path: replace duplicate conditions by functions
3439635 
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
path: replace duplicate conditions by functions
46c9011 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
apapirovski apapirovski approved these changes
maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. path Issues and PRs related to the path subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL