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 (BooleanValue) #22075

Closed
wants to merge 2 commits into from

Conversation

ryzokuken
Copy link
Contributor

Remove all calls to deprecated v8 functions (here:
Value::BooleanValue) 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

@addaleax @hashseed a quick one.

@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 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, although it’s probably easier to just use ->IsTrue(), and I don’t think that would count as a breaking change ;) (edit: at least if we do the coercion in JS first)

@ryzokuken
Copy link
Contributor Author

@addaleax Maybe<bool>::IsTrue is a thing? I didn't know. Also, wouldn't that somehow hurt what we're doing here, as we don't exactly want the same condition to execute? We want it to return to JS-Land if Nothing was returned.

Again, I might be very wrong, but let's look into this.

@addaleax
Copy link
Member

addaleax commented Aug 2, 2018

@ryzokuken value->IsTrue() is a thing, and if we can reasonably expect that value is a boolean already, it’s equivalent to value->BooleanValue(context).ToLocalChecked().

For setAutoPadding, we don’t do any typechecking, but we could make that simplification by using !!ap instead of ap in the JS layer.

For getStringWidth, I’m not sure – they appear unused anyway, beyond a test which accesses internal functions directly (?).

For setNoDelay, we already convert to boolean, so args[0]->IsTrue() is okay to use.

@maclover7
Copy link
Contributor

ping @ryzokuken, looks like this PR needs a rebase :)

@ryzokuken
Copy link
Contributor Author

@maclover7 Will do over the weekend, thanks.

@targos
Copy link
Member

targos commented Aug 26, 2018

Ping @ryzokuken

@targos
Copy link
Member

targos commented Aug 26, 2018

Btw there are two other instances of BooleanValue in src, if you also want to change those:

ryzokuken and others added 2 commits August 29, 2018 15:08
Remove all calls to deprecated v8 functions (here:
Value::BooleanValue) inside the code (src directory only).
@targos
Copy link
Member

targos commented Aug 29, 2018

I fixed the conflicts and handled the remaining cases.
@addaleax @jasnell @ryzokuken PTAL.

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

@ryzokuken
Copy link
Contributor Author

Cannot approve it myself, but LG.

@targos
Copy link
Member

targos commented Aug 31, 2018

CI is green. Can we have at least 1 LG for my additions?

@targos
Copy link
Member

targos commented Sep 1, 2018

@addaleax pretty please?

@addaleax
Copy link
Member

addaleax commented Sep 1, 2018

@targos Sorry, missed the pings – yes, this still LGTM :)

targos pushed a commit to targos/node that referenced this pull request Sep 1, 2018
Remove all calls to deprecated v8 functions (here:
Value::BooleanValue) inside the code (src directory only).

PR-URL: nodejs#22075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member

targos commented Sep 1, 2018

Thank you 😃
Landed in 59e5a39

@targos targos closed this Sep 1, 2018
@ryzokuken
Copy link
Contributor Author

Thanks again, @targos. You're the real MVP.

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

PR-URL: #22075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
Remove all calls to deprecated v8 functions (here:
Value::BooleanValue) inside the code (src directory only).

PR-URL: #22075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Remove all calls to deprecated v8 functions (here:
Value::BooleanValue) inside the code (src directory only).

PR-URL: #22075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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