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, test: fix errors and warnings in VS 2017 #1245

Closed
wants to merge 2 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Dec 4, 2022

This PR has fixes for errors and warnings when running npm test with Visual Studio 2017.

  • napi-inl.h has changes to address the same issue as PR src: refactor call js wrapper #1242
    • fixed use of static keyword for standalone functions. Each static function is duplicated in every cpp file that includes the header. Thus, it produces code that is unnecessary bigger.
  • addon_data.cc - warning about incorrect use of format specifier
  • async_worker.cc - unused variable warning
  • binding.gyp - no warning - just using the same Python quotes as the rest of the code
  • function_reference.cc - warning about possible buffer overflow. The code is changed to avoid memory leak.
  • threadsafe_function.cc, typed_threadsafe_function.cc - warning about napi-inl.h shadowing the tsfn global variable declared in these files.

@vmoroz vmoroz changed the title Fix errors and warnings in VS 2017 src, test: fix errors and warnings in VS 2017 Dec 4, 2022
@KevinEady
Copy link
Contributor

Hi @vmoroz , can you clarify a bit on:

threadsafe_function.cc, typed_threadsafe_function.cc - warning about napi-inl.h shadowing the tsfn global variable declared in these files.

Which tsfn declaration in napi-inl.h is being shadowed?

@vmoroz
Copy link
Member Author

vmoroz commented Dec 9, 2022

Hi @vmoroz , can you clarify a bit on:

threadsafe_function.cc, typed_threadsafe_function.cc - warning about napi-inl.h shadowing the tsfn global variable declared in these files.

Which tsfn declaration in napi-inl.h is being shadowed?

For example, here we define a local variable tsfn. The issue is that the tests define a global variable with the same name. Then the VS2017 issues a warning that the local variable is shadowing the global variable defined in the unit tests. The fix is to change the name of the global variables in tests to s_tsfn.

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.

LGTM!

@mhdawson
Copy link
Member

Kicked off jenkins run here - https://ci.nodejs.org/job/node-test-node-addon-api-new/6750/

Set to only run for windows and Node.js 14.x since I don't think this should affect any other platforms.

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 pushed a commit that referenced this pull request Dec 16, 2022
- fix errors and warnings in VS 2017
- remove incorrect use of static keyword

PR-URL: #1245
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com
@mhdawson
Copy link
Member

Landed in 97736c9

@mhdawson mhdawson closed this Dec 16, 2022
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
- fix errors and warnings in VS 2017
- remove incorrect use of static keyword

PR-URL: nodejs/node-addon-api#1245
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com
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