path: remove unnecessary string copies by tniessen · Pull Request #14438 · nodejs/node

tniessen · 2017-07-23T20:11:27Z

The length of path is known, so path.slice(0, len) === path. Unless I am missing something, these calls are unnecessary.

Partial CI: https://ci.nodejs.org/job/node-test-commit-linuxone/7511/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

path

At this line, `path` has a length of 2, so `path.slice(0, 2) === path`.

mscdex · 2017-07-23T20:51:05Z

Could you add a test for the second change? It seems that line is not currently covered and it would be good to check that the same result is returned before and after this PR.

refack · 2017-07-23T21:14:22Z

@tniessen If you're adding new tests, see if you can cover the if (code === 47/*/*/ || code === 92/*\*/) from L1057

TimothyGu · 2017-07-24T06:05:30Z

lib/path.js

+            ret.root = ret.dir = path;
            return ret;
          }
        }


The

ret.root = ret.dir = path[0];

below can be simplified to

ret.root = ret.dir = path;

as well, AFAICT.

Anyway could you add a comment // `(len <= 1)`

cjihrig

LGTM with a test.

tniessen · 2017-07-24T14:26:17Z

@refack

tniessen · 2017-07-24T15:26:08Z

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

refack

💯

XadillaX

LGTM.

As the length of `path` is known at this point, there is no point in making an exact copy using `slice`. PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

tniessen · 2017-07-26T12:02:54Z

Landed in 2ac0aff f7f590c

CI on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7567/

As the length of `path` is known at this point, there is no point in making an exact copy using `slice`. PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

addaleax · 2017-07-27T22:30:42Z

The second commit (f7f590c) doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know/add the dont-land-on label.

PR-URL: nodejs#14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

Backport-PR-URL: #14521 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

TimothyGu · 2017-08-12T10:23:22Z

I'll handle backporting this to v6.x together with #14440.

Edit: done in #14787

As the length of `path` is known at this point, there is no point in making an exact copy using `slice`. PR-URL: nodejs#14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

Backport-PR-URL: #14787 PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

As the length of `path` is known at this point, there is no point in making an exact copy using `slice`. Backport-PR-URL: #14787 PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

Backport-PR-URL: #14787 PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

As the length of `path` is known at this point, there is no point in making an exact copy using `slice`. Backport-PR-URL: #14787 PR-URL: #14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

PR-URL: nodejs/node#14438 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]>

tniessen added 2 commits July 23, 2017 21:48

path: remove unnecessary string copy

6aff340

At this line, `path` has a length of 2, so `path.slice(0, 2) === path`.

path: remove unnecessary string copy

530f300

tniessen added the path Issues and PRs related to the path subsystem. label Jul 23, 2017

tniessen self-assigned this Jul 23, 2017

tniessen requested a review from refack July 23, 2017 20:11

nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Jul 23, 2017

addaleax approved these changes Jul 23, 2017

View reviewed changes

refack approved these changes Jul 23, 2017

View reviewed changes

jasnell approved these changes Jul 24, 2017

View reviewed changes

lpinca approved these changes Jul 24, 2017

View reviewed changes

TimothyGu reviewed Jul 24, 2017

View reviewed changes

More optimizations

8a07118

cjihrig approved these changes Jul 24, 2017

View reviewed changes

add tests

25d9601

add more detailed tests

fdbffaf

refack approved these changes Jul 24, 2017

View reviewed changes

XadillaX approved these changes Jul 25, 2017

View reviewed changes

TimothyGu approved these changes Jul 25, 2017

View reviewed changes

tniessen closed this Jul 26, 2017

addaleax added the backport-requested-v8.x label Jul 27, 2017

tniessen mentioned this pull request Jul 27, 2017

[v8.x backport] test: increase coverage for path.parse #14521

Closed

addaleax added backported-to-v8.x and removed backport-requested-v8.x labels Jul 27, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

TimothyGu added the backport-requested-v6.x label Aug 12, 2017

TimothyGu mentioned this pull request Aug 12, 2017

[v6.x backport] path.win32 related fixes and tests #14787

Closed

3 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

path: remove unnecessary string copies#14438

path: remove unnecessary string copies#14438
tniessen wants to merge 5 commits intonodejs:masterfrom
tniessen:you-shall-not-slice

tniessen commented Jul 23, 2017

Uh oh!

mscdex commented Jul 23, 2017

Uh oh!

refack commented Jul 23, 2017 •

edited

Loading

Uh oh!

TimothyGu Jul 24, 2017

Uh oh!

refack Jul 24, 2017

Uh oh!

cjihrig left a comment

Uh oh!

tniessen commented Jul 24, 2017

Uh oh!

tniessen commented Jul 24, 2017

Uh oh!

refack left a comment

Uh oh!

XadillaX left a comment

Uh oh!

tniessen commented Jul 26, 2017

Uh oh!

addaleax commented Jul 27, 2017

Uh oh!

TimothyGu commented Aug 12, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

Conversation

tniessen commented Jul 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex commented Jul 23, 2017

Uh oh!

refack commented Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jul 24, 2017

Uh oh!

tniessen commented Jul 24, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

XadillaX left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jul 26, 2017

Uh oh!

addaleax commented Jul 27, 2017

Uh oh!

TimothyGu commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

refack commented Jul 23, 2017 •

edited

Loading

TimothyGu commented Aug 12, 2017 •

edited

Loading