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: replace some deprecated uses of Get() and Set() #24060

Merged
merged 4 commits into from
Nov 7, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 3, 2018

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

@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 Nov 3, 2018
@@ -736,7 +736,7 @@ inline void Environment::SetMethod(v8::Local<v8::Object> that,
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
v8::String::NewFromUtf8(isolate(), name, type).ToLocalChecked();
that->Set(name_string, function);
that->Set(context, name_string, function).FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return if it's empty? Also other FromJust below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joyeecheung I'm not sure that it matters in these two cases.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we can really signal an error here without making SetMethod() itself return a Maybe<>

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax hmm, yeah, the caller can handle that as well, but this is mostly used in Initialize anyways

@danbev
Copy link
Contributor

danbev commented Nov 7, 2018

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

EDIT(cjihrig): CI was yellow

@@ -736,7 +736,7 @@ inline void Environment::SetMethod(v8::Local<v8::Object> that,
const v8::NewStringType type = v8::NewStringType::kInternalized;
v8::Local<v8::String> name_string =
v8::String::NewFromUtf8(isolate(), name, type).ToLocalChecked();
that->Set(name_string, function);
that->Set(context, name_string, function).FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we can really signal an error here without making SetMethod() itself return a Maybe<>

PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@cjihrig cjihrig merged commit 7c64133 into nodejs:master Nov 7, 2018
@cjihrig cjihrig deleted the get branch November 7, 2018 17:43
@danbev danbev mentioned this pull request Nov 8, 2018
2 tasks
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@codebytere
Copy link
Member

@cjihrig should this be backported to v10.x?

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 29, 2018

No, I don't believe so.

addaleax pushed a commit to addaleax/node that referenced this pull request May 30, 2019
PR-URL: nodejs#24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 16, 2019
Backport-PR-URL: #27967
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
Backport-PR-URL: #27967
PR-URL: #24060
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants