Skip to content
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

src: remove calls to deprecated v8 functions (IntegerValue) #22129

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @addaleax @hashseed

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 4, 2018
src/node.cc Outdated
@@ -3123,11 +3123,9 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
return env->ThrowError("Invalid number of arguments.");
}

pid_t pid;
int r;
pid_t pid = args[0].As<Integer>()->Value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add CHECK(args[0]->IsInteger());?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no v8::Value::IsInteger method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I looked at V8's implementation of the Integer class and here is what I found:

  • Integer::Value() can be called on any Number. The value is cast to int64_t.
  • .As<Integer>() can also be called on any Number. The CheckCast does obj->IsNumber().

@@ -172,7 +172,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
return true;
}

int64_t tmp_i = arg->IntegerValue();
int64_t tmp_i = arg.As<Integer>()->Value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ditto, and everywhere else where it’s not obvious that the value already is an integer)

src/node_buffer.cc Outdated Show resolved Hide resolved
int fd = static_cast<int>(stdio->Get(context, fd_key)
.ToLocalChecked()
.As<Integer>()
->Value());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be checked first before converting, I’d say.

lib/buffer.js Outdated
@@ -779,6 +779,7 @@ function bidirectionalIndexOf(buffer, val, byteOffset, encoding, dir) {

function slowIndexOf(buffer, val, byteOffset, encoding, dir) {
var loweredCase = false;
byteOffset = +byteOffset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already done inside of bidirectionalIndexOf

src/node_buffer.cc Show resolved Hide resolved
src/node_buffer.cc Show resolved Hide resolved
@targos targos force-pushed the v8-deprecation-6 branch 2 times, most recently from 2d67e9f to 778ffc7 Compare August 29, 2018 14:28
@targos
Copy link
Member

targos commented Sep 1, 2018

@addaleax I think this one is also ready.

Copying my inline response from yesterday:

So, I looked at V8's implementation of the Integer class and here is what I found:

  • There is no Value::IsInteger() method.
  • Integer::Value() can be called on any Number. The value is cast to int64_t.
  • .As<Integer>() can also be called on any Number. The CheckCast does obj->IsNumber().

@targos
Copy link
Member

targos commented Sep 1, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but casting Number to Integer seems like relying on V8 implementation details, even if it currently works.

I would feel more comfortable if we could add this pattern to the V8 API cctest suite first?

src/node.cc Outdated
@@ -2565,7 +2563,8 @@ static void DebugProcess(const FunctionCallbackInfo<Value>& args) {
goto out;
}

pid = (DWORD) args[0]->IntegerValue();
CHECK(args[0]->IsNumber());
DWORD pid = (DWORD) args[0].As<Integer>()->Value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you happen to be able to test it on Windows without much extra work – I think the cast here is unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It compiles fine, thanks

@targos
Copy link
Member

targos commented Sep 2, 2018

I created a CL to add cctests: https://chromium-review.googlesource.com/c/v8/v8/+/1200983

@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

@targos Thank you :)

@jasnell
Copy link
Member

jasnell commented Sep 4, 2018

Needs a rebase now unfortunately.

Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

Co-authored-by: Michaël Zasso <targos@protonmail.com>
@targos
Copy link
Member

targos commented Sep 4, 2018

Rebased

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018
@targos
Copy link
Member

targos commented Sep 5, 2018

The V8 CL landed. This is ready.

New CI: https://ci.nodejs.org/job/node-test-pull-request/17025/

Edit: Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20550/

@targos
Copy link
Member

targos commented Sep 5, 2018

Landed in f464ac3

@targos targos closed this Sep 5, 2018
targos pushed a commit that referenced this pull request Sep 5, 2018
Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #22129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 5, 2018
Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #22129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 6, 2018
Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #22129
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@refack refack mentioned this pull request Oct 25, 2018
3 tasks
addaleax added a commit to addaleax/node that referenced this pull request Oct 27, 2018
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: nodejs#22129
Fixes: nodejs#23668
MylesBorins pushed a commit that referenced this pull request Oct 29, 2018
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: #22129
Fixes: #23668
addaleax added a commit to addaleax/node that referenced this pull request Dec 20, 2018
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: nodejs#22129
Fixes: nodejs#23668
addaleax added a commit that referenced this pull request Dec 31, 2018
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: #22129
Fixes: #23668

PR-URL: #25154
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jan 1, 2019
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: #22129
Fixes: #23668

PR-URL: #25154
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: nodejs#22129
Fixes: nodejs#23668

PR-URL: nodejs#25154
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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants