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

Suppress NVCC unused variable warnings #2427

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Suppress NVCC unused variable warnings #2427

merged 2 commits into from
Jun 3, 2022

Conversation

ahans
Copy link
Contributor

@ahans ahans commented May 18, 2022

Description

This defines CATCH_INTERNAL_START_WARNINGS_SUPPRESSION and CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION when compiled with nvcc to suppress warnings about unused variables.

GitHub Issues

#2306

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #2427 (524c516) into devel (605a347) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #2427      +/-   ##
==========================================
+ Coverage   91.43%   91.51%   +0.08%     
==========================================
  Files         159      159              
  Lines        7501     7501              
==========================================
+ Hits         6858     6864       +6     
+ Misses        643      637       -6     

Comment on lines 61 to 62
# define CATCH_INTERNAL_START_WARNINGS_SUPPRESSION _Pragma( "nv_diag_suppress 177" )
# define CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION _Pragma( "nv_diag_default 177" )
Copy link
Member

Choose a reason for hiding this comment

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

So, this would work, but wouldn't properly slot into how the macros are used across Catch2. If I am reading the docs correctly, then the correct approach would be something like this

Suggested change
# define CATCH_INTERNAL_START_WARNINGS_SUPPRESSION _Pragma( "nv_diag_suppress 177" )
# define CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION _Pragma( "nv_diag_default 177" )
# define CATCH_INTERNAL_START_WARNINGS_SUPPRESSION _Pragma( "nv_diagnostic push" )
# define CATCH_INTERNAL_STOP_WARNINGS_SUPPRESSION _Pragma( "nv_diagnostic pop" )
# define CATCH_INTERNAL_SUPPRESS_UNUSED_VARIABLE_WARNINGS _Pragma ( "nv_diag_suppress 177" )

Copy link
Contributor Author

@ahans ahans Jun 3, 2022

Choose a reason for hiding this comment

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

Thanks for your comment! Unfortunately, your suggestion makes the warnings come back. The reason is that in the place where the "unused" variables are introduced we do not use CATCH_INTERNAL_SUPPRESS_UNUSED_VARIABLE_WARNINGS, but CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS. Apparently, other compilers than nvcc would emit a different warning here. I see two solutions to this:

  1. Define CATCH_INTERNAL_SUPPRESS_GLOBALS_WARNINGS with the suppression of 177 instead of CATCH_INTERNAL_SUPPRESS_UNUSED_VARIABLE_WARNINGS.
  2. Leave the #defines as you suggested and add CATCH_INTERNAL_SUPPRESS_UNUSED_VARIABLE_WARNINGS to the macros in catch_test_registry.hpp that introduce the variables nvcc complains about.

I updated my PR to implement option 2), since I think that is the cleaner approach. After all, nvcc's warning is about a variable that is "declared but never referenced" and nothing related to global variables. Let me know what you think!

Copy link
Member

Choose a reason for hiding this comment

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

  1. is the right solution, but it might be worth it to open up a bug upstream that a type with opaque constructor triggers unused variable warning. Usually compilers don't warn on this, because they assume that the opaque constructor contains side-effects that the user cares for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants