-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Set -Wno-unused-variable for tests #1682
Conversation
We used assert() a lot in tests and that can cause build breakages in some of the opt builds (since assert() are removed) it's not practical to sprinkle "(void)" everywhere so I think setting this warning option is the best option for now.
do you know why we're not seeing these as issues in the CI bots? release mode would strip the asserts, and we have both -Wall and -Werror enabled so we should be getting these warnings/errors... |
Because |
ah yes, the nuanced distinction between "all" and "everything" (everywhere all at once, presumably). |
Is this missing the CMake part? |
Good point - will send a followup PR. Thanks both! |
#1683 i think does it. |
@@ -18,6 +18,9 @@ TEST_COPTS = [ | |||
# "-Wshorten-64-to-32", | |||
"-Wfloat-equal", | |||
"-fstrict-aliasing", | |||
## assert() are used a lot in tests upstream, which may be optimised out leading to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What "upstream" is this referring to?
I ran into the same problem with unused variables, but only needed one BENCHMARK_MAYBE_UNUSED to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to the test code in this repo (which tends to use assert() rather than GTEST's assertion utilities)
We used assert() a lot in tests and that can cause build breakages in some of the opt builds (since assert() are removed)
it's not practical to sprinkle "(void)" everywhere so I think setting this warning option is the best option for now.