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: return bool on object set and define property #977

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

legendecas
Copy link
Member

If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.

If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
@legendecas
Copy link
Member Author

@nodejs/node-api it will be great for this PR to have reviews, thanks a lot :D

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

Hi @legendecas , just a clarification that changing return type from void to bool is not breaking, correct....? I don't see it as a problem, but perhaps there is something I am missing...

@legendecas
Copy link
Member Author

@KevinEady yes, this is not a breaking change. Existing code will continue to compile and works (and there are still lots of them in the test suites), like https://github.com/nodejs/node-addon-api/blob/main/test/binding.cc#L76.

@legendecas legendecas merged commit 93f1898 into nodejs:main Apr 29, 2021
@legendecas legendecas deleted the object-set branch April 29, 2021 02:49
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Sep 23, 2021
If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
deepakrkris pushed a commit to deepakrkris/node-addon-api that referenced this pull request Oct 15, 2021
If c++ exception is disabled, there is no ergonomic way to detect
failtures on void returning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants