Skip to content

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

Closed
tniessen wants to merge 3 commits intonodejs:masterfrom
tniessen:url-use-foreach-loop
Closed

url: use foreach-style C++ loop#23138
tniessen wants to merge 3 commits intonodejs:masterfrom
tniessen:url-use-foreach-loop

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

nodejs-github-bot commented Sep 28, 2018

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

@TimothyGu TimothyGu Sep 28, 2018

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

@refack refack Sep 28, 2018

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
for (const auto& i : context_.path) {
ret += '/';
ret += *i;
ret += i;
Copy link
Contributor

@refack refack Sep 28, 2018

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

@refack refack Sep 28, 2018

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

@addaleax addaleax Sep 28, 2018

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

@tniessen tniessen Sep 28, 2018

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
for (const auto& i : context_.path) {
ret += '/';
ret += *i;
ret += i;
Copy link
Member

@addaleax addaleax Sep 28, 2018

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

@targos targos Sep 28, 2018

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

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+3 more reviewers

@refack refack refack approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

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