Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upsrc: remove calls to deprecated v8 functions (NumberValue) #22094
Conversation
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
the
C++
label
Aug 2, 2018
addaleax
approved these changes
Aug 2, 2018
src/async_wrap.cc
Outdated
| args[0]->IsNumber() | ||
| ? args[0] | ||
| ->NumberValue(args.GetIsolate()->GetCurrentContext()) | ||
| .ToChecked() |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/async_wrap.cc
Outdated
| wrap->AsyncReset(execution_async_id); | ||
| } | ||
|
|
||
|
|
||
| void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) { | ||
| CHECK(args[0]->IsNumber()); | ||
| AsyncWrap::EmitDestroy( | ||
| Environment::GetCurrent(args), args[0]->NumberValue()); | ||
| Environment::GetCurrent(args), | ||
| args[0]->NumberValue(args.GetIsolate()->GetCurrentContext()).ToChecked()); |
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 2, 2018
Member
We skip validation of the argument in JS land when async hooks checks are disabled – so we probably don’t want ToChecked()?
This comment has been minimized.
This comment has been minimized.
ryzokuken
Aug 3, 2018
Author
Member
Umm, dunno. There's literally a CHECK above, so I thought ToChecked would work here. I mean, it might still crash, sure, but nowhere it didn't crash earlier.
This comment has been minimized.
This comment has been minimized.
addaleax
Aug 3, 2018
Member
Oh, right, I missed that – in that case, we don’t need NumberValue() either and can cast via .As<Number>(), right?
This comment has been minimized.
This comment has been minimized.
jasnell
approved these changes
Aug 3, 2018
cjihrig
approved these changes
Aug 7, 2018
addaleax
approved these changes
Aug 7, 2018
targos
approved these changes
Aug 7, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Landing this. |
This comment has been minimized.
This comment has been minimized.
|
Landed in c6a54af |
ryzokuken
closed this
Aug 7, 2018
ryzokuken
added a commit
that referenced
this pull request
Aug 7, 2018
devsnek
added a commit
to devsnek/node
that referenced
this pull request
Aug 7, 2018
targos
added a commit
that referenced
this pull request
Aug 11, 2018
targos
added a commit
that referenced
this pull request
Aug 11, 2018
This was referenced Aug 16, 2018
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.
ryzokuken commentedAug 2, 2018
Remove all calls to deprecated v8 functions (here:
Value::NumberValue) inside the code (src directory only).
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes/cc @addaleax @hashseed