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 usage of V8 deprecated APIs in node_url.cc #11066

Closed
wants to merge 1 commit into
from

Conversation

Projects
5 participants
@TimothyGu
Member

TimothyGu commented Jan 29, 2017

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@TimothyGu TimothyGu changed the title from src: remove usage of V8 deprecated API in node_url.cc to src: remove usage of V8 soon-to-be-deprecated API in node_url.cc Jan 29, 2017

@TimothyGu TimothyGu changed the title from src: remove usage of V8 soon-to-be-deprecated API in node_url.cc to src: remove usage of V8 deprecated APIs in node_url.cc Jan 29, 2017

Show outdated Hide outdated src/node_url.cc
@@ -1316,7 +1318,8 @@ namespace url {
Utf8Value input(env->isolate(), args[0]);
enum url_parse_state override = kUnknownState;
if (args[1]->IsNumber())
override = (enum url_parse_state)(args[1]->Uint32Value());
override = (enum url_parse_state)(
args[1]->Uint32Value(env->context()).ToChecked());

This comment has been minimized.

@targos

targos Jan 29, 2017

Member

We use FromJust in the code base

@targos

targos Jan 29, 2017

Member

We use FromJust in the code base

This comment has been minimized.

@TimothyGu

TimothyGu Jan 29, 2017

Member

Fixed.

Show outdated Hide outdated src/node_url.cc
@@ -1316,7 +1318,8 @@ namespace url {
Utf8Value input(env->isolate(), args[0]);
enum url_parse_state override = kUnknownState;
if (args[1]->IsNumber())

This comment has been minimized.

@targos

targos Jan 29, 2017

Member

I'd add braces here for clarity now that it expands on two lines

@targos

targos Jan 29, 2017

Member

I'd add braces here for clarity now that it expands on two lines

@addaleax

LGTM with a suggestion

Show outdated Hide outdated src/node_url.cc
override = (enum url_parse_state)(args[1]->Uint32Value());
if (args[1]->IsNumber()) {
override = (enum url_parse_state)(
args[1]->Uint32Value(env->context()).FromJust());

This comment has been minimized.

@addaleax

addaleax Jan 29, 2017

Member

Can you turn this into a reinterpret_cast and rename override to something that’s not a language keyword?

@addaleax

addaleax Jan 29, 2017

Member

Can you turn this into a reinterpret_cast and rename override to something that’s not a language keyword?

This comment has been minimized.

@TimothyGu

TimothyGu Jan 29, 2017

Member

Turns out static_cast works but reinterpret_cast doesn't. override bit fixed.

@TimothyGu

TimothyGu Jan 29, 2017

Member

Turns out static_cast works but reinterpret_cast doesn't. override bit fixed.

@jasnell

LGTM, looks like it needs a rebase

src: remove usage of V8 deprecated API in node_url.cc
Also:

- Avoid using 'override' as variable name
- Use explicit static_cast instead of C-style cast
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 31, 2017

Member

@jasnell, rebased.

@targos, @addaleax, PTAL.

Member

TimothyGu commented Jan 31, 2017

@jasnell, rebased.

@targos, @addaleax, PTAL.

@addaleax

Thanks, LGTM!

@targos

targos approved these changes Jan 31, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 31, 2017

Member

Some failures in that last CI run, trying again: https://ci.nodejs.org/job/node-test-pull-request/6138/

Member

jasnell commented Jan 31, 2017

Some failures in that last CI run, trying again: https://ci.nodejs.org/job/node-test-pull-request/6138/

@TimothyGu TimothyGu added this to Spec Irrelavent / De-facto in WHATWG URL implementation Jan 31, 2017

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Feb 1, 2017

Member

Landed in c7e1be8.

Member

TimothyGu commented Feb 1, 2017

Landed in c7e1be8.

@TimothyGu TimothyGu closed this Feb 1, 2017

@TimothyGu TimothyGu deleted the TimothyGu:url-cpp-deprecated branch Feb 1, 2017

@TimothyGu TimothyGu moved this from Spec Irrelavent / De-facto to Waiting on Spec in WHATWG URL implementation Feb 1, 2017

@TimothyGu TimothyGu moved this from Waiting on Spec to Done in WHATWG URL implementation Feb 1, 2017

TimothyGu added a commit that referenced this pull request Feb 1, 2017

src: remove usage of V8 deprecated API in node_url.cc
Also:

- Avoid using 'override' as variable name
- Use explicit static_cast instead of C-style cast

PR-URL: #11066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 2, 2017

src: remove usage of V8 deprecated API in node_url.cc
Also:

- Avoid using 'override' as variable name
- Use explicit static_cast instead of C-style cast

PR-URL: nodejs#11066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

src: remove usage of V8 deprecated API in node_url.cc
Also:

- Avoid using 'override' as variable name
- Use explicit static_cast instead of C-style cast

PR-URL: nodejs#11066
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment