src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid #46227
+54
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removes the use of vector in EndsInANumber and uses IsIPv4NumberValid instead of parsing the number to check if it is valid. Fixes: nodejs/performance#36 Refs: ada-url/ada#36
nodejs-github-bot
added
c++
|
hello everyone! |
|
@miguelteixeiraa please use the following command: |
|
Done! It hasn't changed that much : / |
|
Yeah, it seems |
|
Of course! |
|
Done! |
anonrig
requested changes
Jan 16, 2023
anonrig
reviewed
Jan 17, 2023
addaleax
reviewed
Jan 17, 2023
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1293/ Results |
github-actions
bot
removed
the
request-ci
|
Okay, windows problems.. not sure what it is but I'm checking |
anonrig
requested changes
Jan 17, 2023
src/node_url.cc
Outdated
| } | ||
|
|
||
| char delimiter = '.'; | ||
| auto pointer_start = input.begin(); |
There was a problem hiding this comment.
If you use std::string_view::iterator it might solve the issues related to std::string_view construct on windows.
There was a problem hiding this comment.
Windows build is still failing : /
D:\a\node\node\src\node_url.cc(465,66): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'std::string_view' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_url.cc(465,66): message : No constructor could take the source type, or constructor overload resolution was ambiguous [D:\a\node\node\libnode.vcxproj]
node_util.cc
There was a problem hiding this comment.
I would like to know if I can go with &(*it); again
There was a problem hiding this comment.
it'd look like this:
std::string_view(&(*pointer_start), pointer_end - pointer_start));
just to make the build pass on windows
There was a problem hiding this comment.
What i understand is that iterators don't need to be pointers although they could be implemented as one. And some implementations of the STL (MSVC++, Mingw) implement interators with special classes (with no pre-difined casts to pointer), so that we cannot use them directly in the string_view constructor that we are using, which needs a charT*.
then we could use * which is overloaded to do what logically mean: convert the iterator type to its pointed-to object, then pass the address of this &(*it).
https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator
https://stackoverflow.com/questions/32654108/c-stdvectoriterator-is-not-a-pointer-why/64910815#64910815
src/node_url.cc
Outdated
|
|
||
| return false; | ||
| return IsIPv4NumberValid( | ||
| std::string_view(&*pointer_start, pointer_end - pointer_start)); |
There was a problem hiding this comment.
This is too hacky (even if it's guaranteed safe).
There was a problem hiding this comment.
I'll try a static_cast<const char*>
There was a problem hiding this comment.
@nodejs/cpp-reviewers any recommendations?
There was a problem hiding this comment.
Yeah: I would definitely recommeng to go back to the original "hacky" implementation. It works and looks correct, even if it is not visually pretty. (I'd also not call this pointer if it's an iterator.) const_cast<char*>(pointer_start) doesn't even really make sense since the string_view constructor takes a const char* argument.
There was a problem hiding this comment.
Yes, I think it only works on the other compilers because their implementations for the iterator are:
std::string_view::iterator' (aka 'const char *')
(I purposely caused an error to catch this 'aka' haha)
anonrig
reviewed
Jan 17, 2023
anonrig
reviewed
Jan 17, 2023
13 tasks
anonrig
approved these changes
Jan 20, 2023
|
microbench: cpu: Apple M1 Pro runtime: node v20.0.0-pre (arm64-darwin) -- with this PR -- |
anonrig
added
review wanted
github-actions
bot
removed
the
request-ci
Are you measuring production build against development build? |
|
Thinking a little more, the microbench doesnt seem a good idea anymore. Sorry for that. Please just ignore (but yes, I guess I am) |
This was referenced Jan 21, 2023
addaleax
approved these changes
Jan 23, 2023
jasnell
approved these changes
Jan 23, 2023
anonrig
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 762a3a8 |
This was referenced Jan 24, 2023
ruyadorno
pushed a commit
that referenced
this pull request
Feb 1, 2023
Removes the use of vector in EndsInANumber and uses IsIPv4NumberValid instead of parsing the number to check if it is valid. Fixes: nodejs/performance#36 Refs: ada-url/ada#36 PR-URL: #46227 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
juanarbol
pushed a commit
that referenced
this pull request
Mar 3, 2023
Removes the use of vector in EndsInANumber and uses IsIPv4NumberValid instead of parsing the number to check if it is valid. Fixes: nodejs/performance#36 Refs: ada-url/ada#36 PR-URL: #46227 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
juanarbol
pushed a commit
that referenced
this pull request
Mar 5, 2023
Removes the use of vector in EndsInANumber and uses IsIPv4NumberValid instead of parsing the number to check if it is valid. Fixes: nodejs/performance#36 Refs: ada-url/ada#36 PR-URL: #46227 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the use of vector in EndsInANumber and uses IsIPv4NumberValid instead of parsing the number to check if it is valid.
Fixes: nodejs/performance#36
Refs: ada-url/ada#36