Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

src: use Local version of ToBoolean() #24924

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
Merged
cjihrig merged 1 commit into from
Dec 11, 2018
Merged

src: use Local version of ToBoolean() #24924

cjihrig 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

nodejs-github-bot commented Dec 9, 2018

@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 <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
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

Reviewers

@fhinkel fhinkel fhinkel approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@joyeecheung joyeecheung joyeecheung 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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.