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 zero-variadic-macro-arguments warning when using clang. #768

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

clalancette
Copy link
Contributor

When building this package with clang, we get a lot of warnings like the following:

must specify at least one argument for '...' parameter of variadic macro

That comes mostly from the TYPED_TEST_SUITE macros in gtest, the signature of which looks like:

Prior to C++20, C++ technically did not allow macros with variadic arguments to be called with nothing for the last variadic part. This is allowed in C++20, but we aren't ready to transition to it yet.

So just add in an unused third "empty" argument anytime we use these variadic arguments. This will quiet the warning on clang and have no effect on operation.

When building this package with clang, we get a lot of
warnings like the following:

must specify at least one argument for '...' parameter of variadic macro

That comes mostly from the TYPED_TEST_SUITE macros in gtest,
the signature of which looks like:

Prior to C++20, C++ technically did not allow macros with variadic
arguments to be called with nothing for the last variadic part.
This is allowed in C++20, but we aren't ready to transition to
it yet.

So just add in an unused third "empty" argument anytime we
use these variadic arguments.  This will quiet the warning on
clang and have no effect on operation.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

clalancette commented Sep 11, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

🤷

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

We ran into this in the gazebo codebase as well and it annoys me deeply.

@clalancette
Copy link
Contributor Author

So cppcheck on Windows is unhappy with this change; it thinks it is a "syntax error". We only see this on Windows since we effectively disabled cppcheck on Linux (the performance with version 2 of cppcheck, which ships in Ubuntu 22.04, is just terrible).

There are two paths forward here:

  1. I could disable cppcheck on Windows for this package. Someday we are going to disable it for the core entirely anyway.
  2. Instead of this patch, I could just disable the warning when using clang. It is kind of a silly error anyway, and since this is only effecting tests I would not feel bad about it.

I think I'm going to go for 2; this will stop being a warning at all in C++20, so I really don't see a downside to doing it.

This reverts commit cdf6746.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette force-pushed the clalancette/zero-variadic-fixes branch from 9d84354 to 1b408f6 Compare September 13, 2023 14:12
@clalancette
Copy link
Contributor Author

And this is now done in 1b408f6. Here's CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Clang Build Status

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

After some offline deliberating, I'm good with this flag. It's an artifact of -Wpedantic and clang and will be resolved in C++20.

Since this is all test code, there is no potential of this leaking to other packages.

@clalancette clalancette merged commit 3491f4f into rolling Sep 13, 2023
@delete-merged-branch delete-merged-branch bot deleted the clalancette/zero-variadic-fixes branch September 13, 2023 14:48
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