-
Notifications
You must be signed in to change notification settings - Fork 2.2k
ci: fix gcc compiler warnings #5523
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
Conversation
cf9cac3
to
b3c6954
Compare
This affects |
Looks like some tests sparsely use includes. Will check later. |
Let's start over and push commits one-by-one. |
b3c6954
to
aedf53b
Compare
The pypy-3.8 build seems to be flaky. |
Yes, very often. |
We need to move to pypy-3.10. Older pypy's aren't supported by upstream. |
@@ -3,6 +3,7 @@ | |||
#pragma once | |||
|
|||
#include "detail/common.h" | |||
#include "detail/type_caster_base.h" |
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.
Sorry I think this is going in the wrong direction:
It brings in a ton of dependencies unrelated to the gil_safe_call_once
functionality.
It's seems counter-productive to turn on compiler warnings just to appease them.
A great step forward would be to refactor the code organization. I'd be very happy if you did that, even if several pieces of code need to be moved around.
If you think that's too much work: Simply disable the warnings for the troublesome existing forward declaration. That'd be a net gain: Having the warnings enabled will nudge us in the right direction when working on new code. The warning suppressions will remind us that we have a code organization cleanup job waiting for an intrepid volunteer.
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.
Fair enough. For restructuring the code, I'm not an expert enough. I will disable the warnings. Switching to draft state for now.
afb5f38
to
a77a7c3
Compare
... introduced with merge of smart_holder branch
This should be ready for review. The failing test seems to be unrelated to my changes. |
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.
Nice, thanks!
Waiting a bit for @henryiii to take a look.
@@ -173,9 +173,6 @@ inline int tp_init_impl(PyObject *, PyObject *, PyObject *) { | |||
// return -1; // Unreachable. | |||
} | |||
|
|||
// The implementation needs the definition of `class cpp_function`. |
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.
Thanks for catching this!
@rhaschke I made a quick change to the Suggested changelog entry. Could you please review, and maybe make it more precise and correct? |
* CI: Fail on any warnings with clang 18 * CI: Fail on any warnings with gcc 13 * Fix cmake's try_compile warning * Guard redundant declarations * ci.yml: fix syntax error * Use PYBIND11_WARNING macros * Fix more redundant declarations ... introduced with merge of smart_holder branch
Description
Removed redundant declarations causing the following warnings (including
pybind11.h
):Suggested changelog entry:
CI testing now includes ``-Wwrite-strings -Wunreachable-code -Wpointer-arith -Wredundant-decls`` in some jobs.