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

fix compilation for Visual Studio 2022 #1492

Merged
merged 2 commits into from
May 7, 2024
Merged

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented May 4, 2024

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

  • async_worker.cc used deprecated std::allocator APIs. Their use is is replaced with APIs that is still valid.
  • There was a strange error in ObjectWrap<T>::WrappedMethod. It was saying that & operation requires an l-value. The issue is fixed by copying method pointer to a local variable before invocation.
  • VS identified that some fields were not initialized. They are changed to initialize with {}.
  • VS identified that some functions must be noexcept. They are augmented with NAPI_NOEXCEPT.

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

@codecov-commenter
Copy link

codecov-commenter commented May 4, 2024

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 ⚠️
Additional details and impacted files
@@           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           

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

@mhdawson
Copy link
Member

mhdawson commented May 6, 2024

@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
Copy link
Member Author

vmoroz commented May 6, 2024

@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
Copy link
Member

mhdawson commented May 6, 2024

@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
Copy link
Member Author

vmoroz commented May 6, 2024

@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.

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
Copy link
Member

mhdawson commented May 7, 2024

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

@mhdawson mhdawson merged commit e011720 into main May 7, 2024
49 checks passed
@KevinEady KevinEady deleted the pr/fix_vs2022_compilation branch May 17, 2024 14:08
@KevinEady KevinEady mentioned this pull request May 17, 2024
@github-actions github-actions bot mentioned this pull request Jun 28, 2024
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.

3 participants