Removing GCC -Wunused-but-set-parameter from pragma block at the top of pybind11.h

[rwgk]

Follow-on to PR #3127, based on results obtained under PR #3125.

Consolidating the new warning suppressions for MSVC & GCC and the existing ignore_unused() function.

Introducing a new macro PYBIND11_WORKAROUND_INCORRECT_GCC_UNUSED_BUT_SET_PARAMETER, similar to the existing one for MSVC.

This PR is mostly trivial, but includes an important commit by @henryiii that is more-or-less independent (but significantly reduces the required workarounds):

218732d

The suggested changelog entry will be in the final PR of this cleanup series.

Worksheet

[rwgk]

For completeness, below is the list of cmake The CXX compiler identifications currently used in the CI workflow.

From the CI results we know for certain that GCC 4 needs the PYBIND11_WORKAROUND_INCORRECT_OLD_GCC_UNUSED_BUT_SET_PARAMETER macro, and GCC 6 does not. GCC 5 is unknown.

I tried to capture this super concisely in the comment, with total line length < 100, but assuming a person caring enough and thinking about it for a minute will be able to figure it out:

#    if defined(__GNUC__) && __GNUC__ <= 5 // 4 is certain, 5 unknown, 6 is certain

AppleClang 12.0.0.12000032
Clang 10.0.0
Clang 13.0.0
Clang 3.6.2
Clang 3.7.1
Clang 3.9.1
Clang 5.0.2
Clang 7.0.1
Clang 9.0.0
GNU 10.3.0
GNU 11.2.0
GNU 4.8.5
GNU 6.3.0
GNU 7.5.0
GNU 8.4.1
GNU 9.3.0
Intel 2021.3.0.20210609
MSVC 19.0.24245.0
MSVC 19.16.27045.0
MSVC 19.29.30040.0
PGI 20.9.0

[henryiii]

"INCORRECT"

I don't see any usage of x, actually - GCC is correct, this is ugly and wrong. It is used in the type expression, but it doesn't need to be - you could just as well use std::declval instead.

Suggested change

	static auto make_move_constructor(const T *x) -> decltype(new T(std::move(*const_cast<T *>(x))), Constructor{}) {
	static auto make_move_constructor(const T *x) -> decltype(&std::declval<T>())), Constructor{}) {

[henryiii]

(And, obviously, remove the "x" and leave the parameter unnamed and remove the lines hiding warnings)

[rwgk]

It builds (after removing extra parenthesis) but fails to run. See below.

I only tried both suggestions together. I'd highly appreciate if we could reserve these side battles for separate PRs. It's fine with me if someone wants to spend the time going above and beyond, but in my position it's difficult to justify spending more time on a very localized, very small enhancement of existing code, only tangentially related to the bigger problem I need to solve.

Running tests in directory "/usr/local/google/home/rwgk/forked/pybind11/tests":
ImportError while loading conftest '/usr/local/google/home/rwgk/forked/pybind11/tests/conftest.py'.
conftest.py:19: in <module>
    import pybind11_tests  # noqa: F401
E   ImportError: return_value_policy = copy, but type MyEnum is non-copyable!

Here is the diff that led to the error above, in case I made a small mistake and someone wants to try it more:

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index a3ce4204..e84e8d99 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -929,18 +929,14 @@ protected:
     /* Only enabled when the types are {copy,move}-constructible *and* when the type
        does not have a private operator new implementation. */
     template <typename T, typename = enable_if_t<is_copy_constructible<T>::value>>
-    static auto make_copy_constructor(const T *x) -> decltype(new T(*x), Constructor{}) {
-        PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(x);
-        PYBIND11_WORKAROUND_INCORRECT_OLD_GCC_UNUSED_BUT_SET_PARAMETER(x);
+    static auto make_copy_constructor(const T *) -> decltype(&std::declval<T>(), Constructor{}) {
         return [](const void *arg) -> void * {
             return new T(*reinterpret_cast<const T *>(arg));
         };
     }
 
     template <typename T, typename = enable_if_t<std::is_move_constructible<T>::value>>
-    static auto make_move_constructor(const T *x) -> decltype(new T(std::move(*const_cast<T *>(x))), Constructor{}) {
-        PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(x);
-        PYBIND11_WORKAROUND_INCORRECT_OLD_GCC_UNUSED_BUT_SET_PARAMETER(x);
+    static auto make_move_constructor(const T *) -> decltype(&std::declval<T>(), Constructor{}) {
         return [](const void *arg) -> void * {
             return new T(std::move(*const_cast<T *>(reinterpret_cast<const T *>(arg))));
         };

[henryiii]

The thing you are replacing is different in the two cases, why would you expect it to become the same expression? new T(*x) is not the same as new T(std::move(*const_cast<T *>(x)). If it makes you feel better, I did exactly the same mistake about 6 times in a row trying to understand why this didn't work. :)

The good news is that by doing it so many times, I have a better idea of what's going on here and was able to simplify the expression a bit - it's easier and more natural when you can make any type you want via declval, rather than having to cast around a preset value.

[henryiii]

Ahh, gh pr checkout 3164 failed, so I modified and pushed to master without noticing it. Reverted, will reset locally and apply to this branch.

[henryiii]

I wondered why git apply didn't work...

[henryiii]

My commit was not really independent - you added a different workaround for this warning, including a new macro PYBIND11_WORKAROUND_INCORRECT_OLD_GCC_UNUSED_BUT_SET_PARAMETER, I fixed the warning instead. Fixing "unused-but-set-parameter" instead of finding a new way to hide it is a perfectly valid way to "remove" a warning and it not unrelated, it fits perfectly under the title.