Skip to content

url: simplify loop in parser #18468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

url: simplify loop in parser #18468

wants to merge 1 commit into from

Conversation

@tniessen
Copy link
Member

I am not sure whether this serves any purpose, the code has not been touched in years.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@tniessen tniessen added url Issues and PRs related to the legacy built-in url module. fast-track PRs that do not need to wait for 48 hours to land. labels Jan 30, 2018
@tniessen
Copy link
Member Author

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018
@tniessen
Copy link
Member Author

CI seems to have some flakes I did not see before, retrying to make sure it is unrelated: https://ci.nodejs.org/job/node-test-pull-request/12849/

@tniessen
Copy link
Member Author

Landed in 26ce1ae.

@tniessen tniessen closed this Jan 31, 2018
tniessen added a commit that referenced this pull request Jan 31, 2018
@tniessen
url: simplify loop in parser
26ce1ae 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@tniessen @MylesBorins
url: simplify loop in parser
1c06909 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@tniessen @MylesBorins
url: simplify loop in parser
c35eacc 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@tniessen @MylesBorins
url: simplify loop in parser
07e4ba2 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@tniessen @MylesBorins
url: simplify loop in parser
6a3bf3c 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed this on v8.x, but opted to not do so on 6.x incase this is a perf related choice. Please let me know if we should backport

MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@tniessen @MylesBorins
url: simplify loop in parser
67f9df9 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@tniessen @MylesBorins
url: simplify loop in parser
5d03c82 
PR-URL: #18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@tniessen @MayaLekova
url: simplify loop in parser
79a7bb1 
PR-URL: nodejs#18468
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 48 hours to land. url Issues and PRs related to the legacy built-in url module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tniessen @MylesBorins @lpinca @TimothyGu @cjihrig @BridgeAR @addaleax