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.

Sign up for GitHub

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

Jump to bottom

src: use Local version of ToBoolean() #24924

Merged
merged 1 commit into from
Dec 11, 2018
Merged

src: use Local version of ToBoolean() #24924

merged 1 commit into from
Dec 11, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 9, 2018

This fixes a deprecation warning.

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1878/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 9, 2018
@targos
Copy link
Member

targos commented Dec 9, 2018

Isn't this removing a check? I'm not familiar with this code but it removes something about a boolean expected?

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 9, 2018

I don't think so. The compiler warning is:

warning: 'ToBoolean' is deprecated: ToBoolean can never throw. Use Local version.

And, the CHECK_TO_TYPE macro that this previously expanded to does have some additional argument checks, but those exact checks are already present in napi_coerce_to_bool() (the function being modified).

@addaleax
Copy link
Member

addaleax commented Dec 9, 2018

@targos I’m not sure which check you’re referring to – CHECK_MAYBE_EMPTY? The reason that the Maybe<> variant of ToBoolean() is deprecated that the conversion can never actually fail, so I think that part is okay?

@targos
Copy link
Member

targos commented Dec 9, 2018

It's just that I see napi_boolean_expected in the old code. It looks like some kind of error that was returned if a non-boolean was passed? I'm on mobile so I'm probably missing a lot of context. If that "error" couldn't be triggered before, it LGTM.

@addaleax
Copy link
Member

addaleax commented Dec 9, 2018

@targos napi_boolean_expected was the status argument of CHECK_TO_TYPE, which in turn was only used for CHECK_MAYBE_EMPTY, so yes, I think we’re good here.

@danbev
Copy link
Contributor

danbev commented Dec 10, 2018

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

@Trott Trott mentioned this pull request Dec 10, 2018
Closed
2 tasks
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 11, 2018

Yellow resume CI: https://ci.nodejs.org/job/node-test-commit/24194/

@cjihrig
src: use Local version of ToBoolean()
91c1f8a 
This fixes a deprecation warning.

PR-URL: nodejs#24924
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit 91c1f8a into nodejs:master Dec 11, 2018
@cjihrig cjihrig deleted the toboolean branch December 11, 2018 21:02
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
@cjihrig @BethGriggs
src: use Local version of ToBoolean()
08c6b21 
This fixes a deprecation warning.

PR-URL: #24924
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
Merged
@beevelop beevelop mentioned this pull request Dec 18, 2018
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@cjihrig @refack
src: use Local version of ToBoolean()
eb30e8b 
This fixes a deprecation warning.

PR-URL: nodejs#24924
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[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
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @nodejs-github-bot @targos @addaleax @danbev @fhinkel @jasnell @joyeecheung @BethGriggs