nodejs / node-addon-api

Module for using Node-API from C++
MIT License
2.15k stars 460 forks source link

fix compilation for Visual Studio 2022 #1492

Closed vmoroz closed 4 months ago

vmoroz commented 5 months ago

Fix PR fixes compilation errors and warnings while compiling code in VS 2022.

BEGIN_COMMIT_OVERRIDE fix: fix compilation for Visual Studio 2022 (#1492) END_COMMIT_OVERRIDE

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 64.71%. Comparing base (4a9d74b) to head (2b52856).

Files Patch % Lines
napi-inl.h 0.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1492 +/- ## ======================================= Coverage 64.71% 64.71% ======================================= Files 3 3 Lines 1981 1981 Branches 687 687 ======================================= Hits 1282 1282 Misses 138 138 Partials 561 561 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mhdawson commented 5 months ago

@vmoroz thanks for the investigation and the PR. One question about the noexcept is that because they are called by other methods that specify noexcept?

I'd have to look closer but adding noexcept I think means it would be breaking to remove those. I understand that if we did make them throw exceptions we'd have to change some other methods to catch those, but the way adding them now would change that to a breaking versus non-breaking change so we should make sure we believe that for those with the additions we believe it would never make sense for them to through an exception. Otherwise we should possibly add a try catch around the calls versus adding the noexcept.

vmoroz commented 5 months ago

@vmoroz thanks for the investigation and the PR. One question about the noexcept is that because they are called by other methods that specify noexcept?

I'd have to look closer but adding noexcept I think means it would be breaking to remove those. I understand that if we did make them throw exceptions we'd have to change some other methods to catch those, but the way adding them now would change that to a breaking versus non-breaking change so we should make sure we believe that for those with the additions we believe it would never make sense for them to through an exception. Otherwise we should possibly add a try catch around the calls versus adding the noexcept.

@mhdawson , it were the recommendations from Visual Studio. The move constructors and assignment operators are typically must be noexcept. In other cases VS saw that methods never through and suggested to make them noexcept. These changes were just some "good to have". I can remove them from this PR to keep the bare minimum that fixes only compilation errors.

mhdawson commented 5 months ago

@vmoroz I'd say lets keep this PR to just what is needed to fix the compile failures, and then we can look more closely at the good to haves :)

vmoroz commented 5 months ago

@mhdawson , I have removed all newly added NAPI_NOEXCEPT, but I kept the initialization of previously uninitialized variables. They do not cause compilation errors, but I believe they are quite important to avoid undefined behavior.

mhdawson commented 4 months ago

@vmoroz many thanks. I'm going to go ahead an land this so that we can get the cross platform CI testing green again.