-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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.
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
lib: restate the code to use primordials #36552
Conversation
dfa522c
to
07855be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also replace those please:
node/lib/internal/validators.js
Line 66 in f28715d
if (!octalReg.test(value)) { |
node/lib/internal/validators.js
Line 201 in f28715d
(typeof port === 'string' && port.trim().length === 0) || |
@aduh95 - yes, I modified the changes. PTAL |
Commit message should indicate where we are using more primordials. Maybe this?
|
Removed |
It looks like the benchmark is for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if no benchmark or CI issues.
@Trott - Thanks for the review. without fully knowing it, I left the default values in jenkins interface. |
Benchmark result : https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/782/console
is this performance result within allowable limits? |
81f7df1
to
5188055
Compare
PR-URL: nodejs#36552 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
64fbf93
to
0538749
Compare
Landed in 0538749 |
PR-URL: #36552 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes