Skip to content
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

url: use foreach-style C++ loop #23138

Closed
wants to merge 3 commits into from
Closed

url: use foreach-style C++ loop #23138

wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 28, 2018 •
edited
Loading

Change URL::parse to use a range-based for loop for better readability.

CI on commit before opening PR: https://ci.nodejs.org/job/node-test-commit/21840/

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

@nodejs-github-bot
Copy link
Collaborator

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1047/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Sep 28, 2018
TimothyGu
TimothyGu approved these changes Sep 28, 2018
View reviewed changes
src/node_url.h Outdated
@@ -159,9 +159,9 @@ class URL {

std::string path() const {
std::string ret;
for (auto i = context_.path.begin(); i != context_.path.end(); i++) {
for (const auto& i : context_.path) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace auto with std::string?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would ask not to.
As per the discussion in #23028

refack
refack reviewed Sep 28, 2018
View reviewed changes
src/node_url.h Outdated
ret += '/';
ret += *i;
ret += i;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two possible minor improvements.
One is to use stringstream
The other is to hint to the compiler the the / always goes with the next part:

ret += '/' + i;

YMMV

Copy link
Contributor

Choose a reason for hiding this comment

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

Theoretically this should be the best solution, but I'm not sure I like it:

  return std::accumulate(cbegin(_path), cend(_path), std::string(), 
                         [](auto const& a, auto const& b) { return a + '/' + b; });

Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer std::accumulate over stringstream but otherwise the suggestions in these two comments seem fine (as does the current state of the PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind using a range-based for loop was to improve readability, and using accumulate for such a trivial thing seems like doing the opposite.

addaleax
addaleax approved these changes Sep 28, 2018
View reviewed changes
src/node_url.h Outdated
ret += '/';
ret += *i;
ret += i;
Copy link
Member

Choose a reason for hiding this comment

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

I’d prefer std::accumulate over stringstream but otherwise the suggestions in these two comments seem fine (as does the current state of the PR)

@tniessen tniessen force-pushed the url-use-foreach-loop branch from bcad6b4 to b3f40ee Compare September 28, 2018 09:37
@tniessen tniessen force-pushed the url-use-foreach-loop branch from b3f40ee to 2440d9c Compare September 28, 2018 09:37
targos
targos reviewed Sep 28, 2018
View reviewed changes
src/node_url.h Outdated
for (auto i = context_.path.begin(); i != context_.path.end(); i++) {
ret += '/';
ret += *i;
for (const std::string& i : context_.path) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename it to something like path_element?
To me, i means "index" and that's how it's always used in JS.

@tniessen
Copy link
Member Author

tniessen commented Sep 29, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/17504/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17533/

@addaleax
Copy link
Member

addaleax commented Oct 1, 2018

Landed in 1d9ec04

@addaleax addaleax closed this Oct 1, 2018
addaleax pushed a commit that referenced this pull request Oct 1, 2018
@tniessen @addaleax
url: use foreach-style C++ loop
1d9ec04 
PR-URL: #23138
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
@tniessen @targos
url: use foreach-style C++ loop
24c3406 
PR-URL: #23138
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@joyeecheung joyeecheung mentioned this pull request Oct 2, 2018
Closed
targos pushed a commit that referenced this pull request Oct 3, 2018
@tniessen @targos
url: use foreach-style C++ loop
d22ee2c 
PR-URL: #23138
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
This was referenced Oct 11, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tniessen @nodejs-github-bot @addaleax @refack @jasnell @thefourtheye @TimothyGu @targos @cjihrig @joyeecheung