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

[BUG]: token pasting of ',' and __VA_ARGS__ is a GNU extension / pedantic warnings #5310

Closed
2 of 3 tasks
N-Wouda opened this issue Aug 14, 2024 · 3 comments · Fixed by #5322
Closed
2 of 3 tasks

[BUG]: token pasting of ',' and __VA_ARGS__ is a GNU extension / pedantic warnings #5310

N-Wouda opened this issue Aug 14, 2024 · 3 comments · Fixed by #5322
Labels
triage New bug, unverified

Comments

@N-Wouda
Copy link

N-Wouda commented Aug 14, 2024

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

2.13.3

Problem description

The most recent release uses , ##__VA_ARGS__ in common.h. This is not technically in the standard (see e.g. here), but most compilers support it. Clang, however, complains about its use when setting -Wpedantic. I can work around this by either not passing -Wpedantic, or passing -Wno-gnu-zero-variadic-macro-arguments, but I'd rather not do that if it can at all be avoided.

Consider the attached example, test.cpp. Compiling it with

clang -Wall -Wextra -Wpedantic -Werror -shared -std=c++20 -fPIC $(python -m pybind11 --includes) test.cpp -o test.so

produces the following:

In file included from test.cpp:1:
In file included from .venv/lib/python3.11/site-packages/pybind11/include/pybind11/pybind11.h:13:
In file included from .venv/lib/python3.11/site-packages/pybind11/include/pybind11/detail/class.h:12:
In file included from .venv/lib/python3.11/site-packages/pybind11/include/pybind11/attr.h:13:
.venv/lib/python3.11/site-packages/pybind11/include/pybind11/detail/common.h:493:57: error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
            &PYBIND11_CONCAT(pybind11_module_def_, name),
  \
                                                        ^
1 error generated.

It concerns the following code snippet:

&PYBIND11_CONCAT(pybind11_module_def_, name), \
##__VA_ARGS__); \

I am not sure if pybind needs to consider this a bug or even an issue. But the previous versions of pybind (2.12 and below) worked without a hitch when setting the -Wpedantic option, so I figured I'd let you know about this here.

Reproducible example code

#include <pybind11/pybind11.h>

int test()
{
    return 42;
}

PYBIND11_MODULE(module, m)
{
    m.def("test", &test);
}

Is this a regression? Put the last known working version here if it is.

2.12.0

@N-Wouda N-Wouda added the triage New bug, unverified label Aug 14, 2024
@henryiii
Copy link
Collaborator

PYBIND11_MODULE(module, m, py::mod_gil_not_used()) would fix it, I think?

We probably should push and pop this warning, then.

@henryiii
Copy link
Collaborator

I don't think we can fully hide the warning inside a macro. There are actually three cases, the others have been in the code base forever but are less common. In #5322 I've managed to get it working with the flag for C++20 mode. Before C++20, GCC's warning isn't suppressible, since it's not categorized (it's just -Werror), and GCC doesn't seem to be reliable about allowing it to be suppressed as -Wpedantic. And Clang can produce another error code here before C++20, which probably isn't in older compilers, so I think supporting C++20 for pedantic is probably fine.

@N-Wouda
Copy link
Author

N-Wouda commented Aug 21, 2024

Thanks @henryiii! In my use case we compile everything in C++20 mode, so this would solve my particular issue. As you mention it is difficult to backport this to earlier C++ modes, I think supporting 20 and up should suffice for most (new) projects. In any case I'm a big fan of supporting -Wpedantic builds 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage New bug, unverified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants