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

feat: allow catching all exceptions #1593

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

KevinEady
Copy link
Contributor

@KevinEady KevinEady commented Oct 18, 2024

Add new compile-time define (NODE_ADDON_API_CPP_EXCEPTIONS_ALL) to enable catching all exceptions. This must be used in conjunction with NODE_ADDON_API_CPP_EXCEPTIONS.

Rename the NAPI_ prefixed exception preprocessor definitions (NAPI_CPP_EXCEPTIONS and NAPI_DISABLE_CPP_EXCEPTIONS) to be prefixed with our standard NODE_ADDON_API_ prefix in a backwards-compatible way.

Fixes: #1555

@KevinEady
Copy link
Contributor Author

I'd like to discuss the naming of the new define NAPI_CPP_EXCEPTIONS_ALL ... to go with convention it should be NODE_ADDON_API_CPP_EXCEPTIONS_ALL.

We can use NODE_ADDON_API_CPP_EXCEPTIONS_ALL and introduce NODE_ADDON_API_CPP_EXCEPTIONS as a (backwards-compatible) replacement for NAPI_CPP_EXCEPTIONS_ALL ?

@KevinEady
Copy link
Contributor Author

We discuss in the 18 Oct Node-API meeting:

  • Use the NODE_ADDON_API_ prefix on the defines
  • Should the "catch all" be the new default?
    • What about some best practices doc? We also mentioned this with the requiring basic finalizers option

napi-inl.h Show resolved Hide resolved
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Is this branch in sync with the main?

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 66.29%. Comparing base (98aae33) to head (df4e6b1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
napi-inl.h 85.00% 1 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1593      +/-   ##
==========================================
- Coverage   66.35%   66.29%   -0.07%     
==========================================
  Files           3        3              
  Lines        2143     2154      +11     
  Branches      703      708       +5     
==========================================
+ Hits         1422     1428       +6     
  Misses        150      150              
- Partials      571      576       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KevinEady KevinEady marked this pull request as ready for review November 7, 2024 23:35
@KevinEady
Copy link
Contributor Author

Hi @vmoroz @mhdawson @legendecas @gabrielschulhof , I believe this is ready.

napi-inl.h Outdated Show resolved Hide resolved
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Looks good to me! A minor suggestion...

Co-authored-by: Chengzhong Wu <[email protected]>
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit c679f6f into nodejs:main Nov 15, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

std::exception catching once more
5 participants