Tighten up warnings #315
Closed
Tighten up warnings #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhdawson
reviewed
Sep 6, 2018
| @@ -3207,7 +3218,7 @@ inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) { | |||
| } | |||
|
|
|||
| inline void AsyncWorker::OnWorkComplete( | |||
| napi_env env, napi_status status, void* this_pointer) { | |||
| napi_env /*env*/, napi_status status, void* this_pointer) { | |||
There was a problem hiding this comment.
Why is it necessary to comment out some of the parameter names like this?
There was a problem hiding this comment.
Parameter of a function definition, that is not used in function body generate warning with -Wunused-parameter enabled.
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Since library is header-only, even function definitions will be compiled with library user (as opposed to library author) flags. If library user has this warning enabled one will get warnings coming from library code, and to fix/hide them one will have to disable warnings for every compilation unit that includes this library, or wrap include with #pragma push-ignore-pop dance. Or fork/vendor library and fix warnings.
|
CI run to test across platforms: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api/653/ |
| @@ -40,6 +43,14 @@ namespace details { | |||
| return __VA_ARGS__; \ | |||
| } | |||
|
|
|||
| // Usually ony could use NAPI_THROW_IF_FAILED(env, status, void()), | |||
There was a problem hiding this comment.
nit: How about we re-phrase this to
// We need a _VOID version of this macro to avoid warnings resulting from
// leaving the NAPI_THROW_IF_FAILED `...` argument empty.
mhdawson
approved these changes
Sep 10, 2018
gabrielschulhof
pushed a commit
that referenced
this pull request
Sep 10, 2018
PR-URL: #315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
|
Landed in 622ffae. |
gabrielschulhof
pushed a commit
to gabrielschulhof/node-addon-api
that referenced
this pull request
Sep 13, 2018
PR-URL: nodejs#315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
mhdawson
added a commit
to mhdawson/node-addon-api
that referenced
this pull request
Sep 18, 2018
Since original submit for nodejs#292 warnings were tightened through nodejs#315 causing the bigint test to fail to compile Fix the compile failure
kevindavies8
added a commit
to kevindavies8/node-addon-api-Develop
that referenced
this pull request
Aug 24, 2022
PR-URL: nodejs/node-addon-api#315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
kevindavies8
added a commit
to kevindavies8/node-addon-api-Develop
that referenced
this pull request
Aug 24, 2022
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
Marlyfleitas
added a commit
to Marlyfleitas/node-api-addon-Development
that referenced
this pull request
Aug 26, 2022
PR-URL: nodejs/node-addon-api#315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
Marlyfleitas
added a commit
to Marlyfleitas/node-api-addon-Development
that referenced
this pull request
Aug 26, 2022
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
wroy7860
added a commit
to wroy7860/addon-api-benchmark-node
that referenced
this pull request
Sep 19, 2022
PR-URL: nodejs/node-addon-api#315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
wroy7860
added a commit
to wroy7860/addon-api-benchmark-node
that referenced
this pull request
Sep 19, 2022
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
johnfrench3
pushed a commit
to johnfrench3/node-addon-api-git
that referenced
this pull request
Aug 11, 2023
PR-URL: nodejs/node-addon-api#315 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
johnfrench3
pushed a commit
to johnfrench3/node-addon-api-git
that referenced
this pull request
Aug 11, 2023
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enable more warnings in tests and mark them as errors, to ensure that headers would not interfere with build environment of project using this library.