url: use foreach-style C++ loop#23138
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
src/node_url.h Outdated
| std::string path() const{ | ||
| std::string ret; | ||
| for (auto i = context_.path.begin(); i != context_.path.end(); i++){ | ||
| for (constauto& i : context_.path){ |
There was a problem hiding this comment.
Can we replace auto with std::string?
There was a problem hiding this comment.
I would ask not to.
As per the discussion in #23028
src/node_url.h Outdated
| for (constauto& i : context_.path){ | ||
| ret += '/'; | ||
| ret += *i; | ||
| ret += i; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}); 
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
src/node_url.h Outdated
| for (constauto& i : context_.path){ | ||
| ret += '/'; | ||
| ret += *i; | ||
| ret += i; |
There was a problem hiding this comment.
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)
tniessenforce-pushed the url-use-foreach-loop branch from bcad6b4 to b3f40eeComparetniessenforce-pushed the url-use-foreach-loop branch from b3f40ee to 2440d9cCompare
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){ |
There was a problem hiding this comment.
Maybe rename it to something like path_element?
To me, i means "index" and that's how it's always used in JS.
addaleax commented Oct 1, 2018
Landed in 1d9ec04 |
PR-URL: #23138 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: #23138 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: #23138 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
10 participants
Change
URL::parseto 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), orvcbuild test(Windows) passes