From 8cf1cdbc96ac326ff6d64a36ea291472f74c016f Mon Sep 17 00:00:00 2001 From: Francesco Ballarin Date: Sat, 5 Oct 2024 15:40:14 +0200 Subject: [PATCH 01/21] Incomplete attempt to address regression introduced in #5381 --- include/pybind11/cast.h | 4 +-- tests/test_kwargs_and_defaults.cpp | 46 ++++++++++++++++++++++++++++++ tests/test_kwargs_and_defaults.py | 12 ++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e3c8b9f659..ebbe6c3642 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1570,9 +1570,9 @@ class argument_loader { using indices = make_index_sequence; template - using argument_is_args = std::is_base_of>; + using argument_is_args = all_of>, negation>>; template - using argument_is_kwargs = std::is_base_of>; + using argument_is_kwargs = all_of>, negation>>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 09036ccd51..a1a57ded6b 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -21,6 +21,33 @@ class ArgsSubclass : public py::args { class KWArgsSubclass : public py::kwargs { using py::kwargs::kwargs; }; +class MoveOrCopyInt { +public: + MoveOrCopyInt() { print_default_created(this); } + explicit MoveOrCopyInt(int v) : value{v} { print_created(this, value); } + MoveOrCopyInt(MoveOrCopyInt &&m) noexcept { + print_move_created(this, m.value); + std::swap(value, m.value); + } + MoveOrCopyInt &operator=(MoveOrCopyInt &&m) noexcept { + print_move_assigned(this, m.value); + std::swap(value, m.value); + return *this; + } + MoveOrCopyInt(const MoveOrCopyInt &c) { + print_copy_created(this, c.value); + // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) + value = c.value; + } + MoveOrCopyInt &operator=(const MoveOrCopyInt &c) { + print_copy_assigned(this, c.value); + value = c.value; + return *this; + } + ~MoveOrCopyInt() { print_destroyed(this); } + + int value; +}; namespace pybind11 { namespace detail { template <> @@ -31,6 +58,19 @@ template <> struct handle_type_name { static constexpr auto name = const_name("**KWArgs"); }; +template <> +struct type_caster { + PYBIND11_TYPE_CASTER(MoveOrCopyInt*, const_name("MoveOrCopyInt")); + bool load(handle src, bool) { + auto as_class = MoveOrCopyInt(src.cast()); + value = &as_class; + return true; + } + static handle cast(int v, return_value_policy r, handle p) { + auto as_class = MoveOrCopyInt(v); + return pybind11::handle(as_class, r, p); + } +}; } // namespace detail } // namespace pybind11 @@ -348,4 +388,10 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { [](const ArgsSubclass &args, const KWArgsSubclass &kwargs) { return py::make_tuple(args, kwargs); }); + + // Test that support for args and kwargs subclasses skips checking arguments passed in as pointers + m.def("args_kwargs_subclass_function_with_pointer_arg", + [](MoveOrCopyInt* pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { + return py::make_tuple(pointer->value, args, kwargs); + }); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index e3f758165c..8bc086c2cb 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -21,6 +21,10 @@ def test_function_signatures(doc): doc(m.args_kwargs_subclass_function) == "args_kwargs_subclass_function(*Args, **KWArgs) -> tuple" ) + assert ( + doc(m.args_kwargs_subclass_function_with_pointer_arg) + == "args_kwargs_subclass_function_with_pointer_arg(arg0: NotArgsOrKWArgsClass, *Args, **KWArgs) -> tuple" + ) assert ( doc(m.KWClass.foo0) == "foo0(self: m.kwargs_and_defaults.KWClass, arg0: int, arg1: float) -> None" @@ -103,6 +107,7 @@ def test_arg_and_kwargs(): kwargs = {"arg3": "a3", "arg4": 4} assert m.args_kwargs_function(*args, **kwargs) == (args, kwargs) assert m.args_kwargs_subclass_function(*args, **kwargs) == (args, kwargs) + assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == (10, args, kwargs) def test_mixed_args_and_kwargs(msg): @@ -424,6 +429,13 @@ def test_args_refcount(): ) assert refcount(myval) == expected + assert m.args_kwargs_subclass_function_with_pointer_arg(7, 8, myval, a=1, b=myval) == ( + 7, + (8, myval), + {"a": 1, "b": myval}, + ) + assert refcount(myval) == expected + exp3 = refcount(myval, myval, myval) assert m.args_refcount(myval, myval, myval) == (exp3, exp3, exp3) assert refcount(myval) == expected From 9d107d2f751d76b2c26e90cd08d2ac163022c873 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 5 Oct 2024 13:45:38 +0000 Subject: [PATCH 02/21] style: pre-commit fixes --- include/pybind11/cast.h | 6 ++++-- tests/test_kwargs_and_defaults.cpp | 7 ++++--- tests/test_kwargs_and_defaults.py | 10 ++++++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ebbe6c3642..c2f14ebc6e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1570,9 +1570,11 @@ class argument_loader { using indices = make_index_sequence; template - using argument_is_args = all_of>, negation>>; + using argument_is_args + = all_of>, negation>>; template - using argument_is_kwargs = all_of>, negation>>; + using argument_is_kwargs + = all_of>, negation>>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index a1a57ded6b..cbaebf0d39 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -60,7 +60,7 @@ struct handle_type_name { }; template <> struct type_caster { - PYBIND11_TYPE_CASTER(MoveOrCopyInt*, const_name("MoveOrCopyInt")); + PYBIND11_TYPE_CASTER(MoveOrCopyInt *, const_name("MoveOrCopyInt")); bool load(handle src, bool) { auto as_class = MoveOrCopyInt(src.cast()); value = &as_class; @@ -389,9 +389,10 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { return py::make_tuple(args, kwargs); }); - // Test that support for args and kwargs subclasses skips checking arguments passed in as pointers + // Test that support for args and kwargs subclasses skips checking arguments passed in as + // pointers m.def("args_kwargs_subclass_function_with_pointer_arg", - [](MoveOrCopyInt* pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { + [](MoveOrCopyInt *pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { return py::make_tuple(pointer->value, args, kwargs); }); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 8bc086c2cb..78bbb2a6bc 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -107,7 +107,11 @@ def test_arg_and_kwargs(): kwargs = {"arg3": "a3", "arg4": 4} assert m.args_kwargs_function(*args, **kwargs) == (args, kwargs) assert m.args_kwargs_subclass_function(*args, **kwargs) == (args, kwargs) - assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == (10, args, kwargs) + assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == ( + 10, + args, + kwargs, + ) def test_mixed_args_and_kwargs(msg): @@ -429,7 +433,9 @@ def test_args_refcount(): ) assert refcount(myval) == expected - assert m.args_kwargs_subclass_function_with_pointer_arg(7, 8, myval, a=1, b=myval) == ( + assert m.args_kwargs_subclass_function_with_pointer_arg( + 7, 8, myval, a=1, b=myval + ) == ( 7, (8, myval), {"a": 1, "b": myval}, From b508a071746a17c078b4b54063c723d623f63aa5 Mon Sep 17 00:00:00 2001 From: Francesco Ballarin Date: Sun, 6 Oct 2024 09:41:46 +0200 Subject: [PATCH 03/21] Revert "style: pre-commit fixes" This reverts commit 9d107d2f751d76b2c26e90cd08d2ac163022c873. --- include/pybind11/cast.h | 6 ++---- tests/test_kwargs_and_defaults.cpp | 7 +++---- tests/test_kwargs_and_defaults.py | 10 ++-------- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c2f14ebc6e..ebbe6c3642 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1570,11 +1570,9 @@ class argument_loader { using indices = make_index_sequence; template - using argument_is_args - = all_of>, negation>>; + using argument_is_args = all_of>, negation>>; template - using argument_is_kwargs - = all_of>, negation>>; + using argument_is_kwargs = all_of>, negation>>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index cbaebf0d39..a1a57ded6b 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -60,7 +60,7 @@ struct handle_type_name { }; template <> struct type_caster { - PYBIND11_TYPE_CASTER(MoveOrCopyInt *, const_name("MoveOrCopyInt")); + PYBIND11_TYPE_CASTER(MoveOrCopyInt*, const_name("MoveOrCopyInt")); bool load(handle src, bool) { auto as_class = MoveOrCopyInt(src.cast()); value = &as_class; @@ -389,10 +389,9 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { return py::make_tuple(args, kwargs); }); - // Test that support for args and kwargs subclasses skips checking arguments passed in as - // pointers + // Test that support for args and kwargs subclasses skips checking arguments passed in as pointers m.def("args_kwargs_subclass_function_with_pointer_arg", - [](MoveOrCopyInt *pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { + [](MoveOrCopyInt* pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { return py::make_tuple(pointer->value, args, kwargs); }); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 78bbb2a6bc..8bc086c2cb 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -107,11 +107,7 @@ def test_arg_and_kwargs(): kwargs = {"arg3": "a3", "arg4": 4} assert m.args_kwargs_function(*args, **kwargs) == (args, kwargs) assert m.args_kwargs_subclass_function(*args, **kwargs) == (args, kwargs) - assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == ( - 10, - args, - kwargs, - ) + assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == (10, args, kwargs) def test_mixed_args_and_kwargs(msg): @@ -433,9 +429,7 @@ def test_args_refcount(): ) assert refcount(myval) == expected - assert m.args_kwargs_subclass_function_with_pointer_arg( - 7, 8, myval, a=1, b=myval - ) == ( + assert m.args_kwargs_subclass_function_with_pointer_arg(7, 8, myval, a=1, b=myval) == ( 7, (8, myval), {"a": 1, "b": myval}, From 95e03253ac2f997ee656bf56887359176897d91f Mon Sep 17 00:00:00 2001 From: Francesco Ballarin Date: Sun, 6 Oct 2024 09:42:01 +0200 Subject: [PATCH 04/21] Revert "Incomplete attempt to address regression introduced in #5381" This reverts commit 8cf1cdbc96ac326ff6d64a36ea291472f74c016f. --- include/pybind11/cast.h | 4 +-- tests/test_kwargs_and_defaults.cpp | 46 ------------------------------ tests/test_kwargs_and_defaults.py | 12 -------- 3 files changed, 2 insertions(+), 60 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ebbe6c3642..e3c8b9f659 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1570,9 +1570,9 @@ class argument_loader { using indices = make_index_sequence; template - using argument_is_args = all_of>, negation>>; + using argument_is_args = std::is_base_of>; template - using argument_is_kwargs = all_of>, negation>>; + using argument_is_kwargs = std::is_base_of>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index a1a57ded6b..09036ccd51 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -21,33 +21,6 @@ class ArgsSubclass : public py::args { class KWArgsSubclass : public py::kwargs { using py::kwargs::kwargs; }; -class MoveOrCopyInt { -public: - MoveOrCopyInt() { print_default_created(this); } - explicit MoveOrCopyInt(int v) : value{v} { print_created(this, value); } - MoveOrCopyInt(MoveOrCopyInt &&m) noexcept { - print_move_created(this, m.value); - std::swap(value, m.value); - } - MoveOrCopyInt &operator=(MoveOrCopyInt &&m) noexcept { - print_move_assigned(this, m.value); - std::swap(value, m.value); - return *this; - } - MoveOrCopyInt(const MoveOrCopyInt &c) { - print_copy_created(this, c.value); - // NOLINTNEXTLINE(cppcoreguidelines-prefer-member-initializer) - value = c.value; - } - MoveOrCopyInt &operator=(const MoveOrCopyInt &c) { - print_copy_assigned(this, c.value); - value = c.value; - return *this; - } - ~MoveOrCopyInt() { print_destroyed(this); } - - int value; -}; namespace pybind11 { namespace detail { template <> @@ -58,19 +31,6 @@ template <> struct handle_type_name { static constexpr auto name = const_name("**KWArgs"); }; -template <> -struct type_caster { - PYBIND11_TYPE_CASTER(MoveOrCopyInt*, const_name("MoveOrCopyInt")); - bool load(handle src, bool) { - auto as_class = MoveOrCopyInt(src.cast()); - value = &as_class; - return true; - } - static handle cast(int v, return_value_policy r, handle p) { - auto as_class = MoveOrCopyInt(v); - return pybind11::handle(as_class, r, p); - } -}; } // namespace detail } // namespace pybind11 @@ -388,10 +348,4 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { [](const ArgsSubclass &args, const KWArgsSubclass &kwargs) { return py::make_tuple(args, kwargs); }); - - // Test that support for args and kwargs subclasses skips checking arguments passed in as pointers - m.def("args_kwargs_subclass_function_with_pointer_arg", - [](MoveOrCopyInt* pointer, const ArgsSubclass &args, const KWArgsSubclass &kwargs) { - return py::make_tuple(pointer->value, args, kwargs); - }); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 8bc086c2cb..e3f758165c 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -21,10 +21,6 @@ def test_function_signatures(doc): doc(m.args_kwargs_subclass_function) == "args_kwargs_subclass_function(*Args, **KWArgs) -> tuple" ) - assert ( - doc(m.args_kwargs_subclass_function_with_pointer_arg) - == "args_kwargs_subclass_function_with_pointer_arg(arg0: NotArgsOrKWArgsClass, *Args, **KWArgs) -> tuple" - ) assert ( doc(m.KWClass.foo0) == "foo0(self: m.kwargs_and_defaults.KWClass, arg0: int, arg1: float) -> None" @@ -107,7 +103,6 @@ def test_arg_and_kwargs(): kwargs = {"arg3": "a3", "arg4": 4} assert m.args_kwargs_function(*args, **kwargs) == (args, kwargs) assert m.args_kwargs_subclass_function(*args, **kwargs) == (args, kwargs) - assert m.args_kwargs_subclass_function_with_pointer_arg(10, *args, **kwargs) == (10, args, kwargs) def test_mixed_args_and_kwargs(msg): @@ -429,13 +424,6 @@ def test_args_refcount(): ) assert refcount(myval) == expected - assert m.args_kwargs_subclass_function_with_pointer_arg(7, 8, myval, a=1, b=myval) == ( - 7, - (8, myval), - {"a": 1, "b": myval}, - ) - assert refcount(myval) == expected - exp3 = refcount(myval, myval, myval) assert m.args_refcount(myval, myval, myval) == (exp3, exp3, exp3) assert refcount(myval) == expected From 8cbca65cab0ef952de2e6eeab7f8ee15990e74bf Mon Sep 17 00:00:00 2001 From: Francesco Ballarin Date: Sun, 6 Oct 2024 09:49:17 +0200 Subject: [PATCH 05/21] Simpler fix for the regression introduced in #5381 --- include/pybind11/cast.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e3c8b9f659..1901a3ab5d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1569,10 +1569,16 @@ template class argument_loader { using indices = make_index_sequence; + // See PR #5396 for the discussion that led to using + // any_of, std::is_base_of<...>> + // instead of simply + // std::is_base_of<...> template - using argument_is_args = std::is_base_of>; + using argument_is_args = any_of< + std::is_same, args>, std::is_base_of>>; template - using argument_is_kwargs = std::is_base_of>; + using argument_is_kwargs = any_of< + std::is_same, kwargs>, std::is_base_of>>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); From 4c87a236736504e81c1801cc6523223ce84e2d2a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 6 Oct 2024 08:12:27 +0000 Subject: [PATCH 06/21] style: pre-commit fixes --- include/pybind11/cast.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1901a3ab5d..372aa8a80e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1574,11 +1574,11 @@ class argument_loader { // instead of simply // std::is_base_of<...> template - using argument_is_args = any_of< - std::is_same, args>, std::is_base_of>>; + using argument_is_args + = any_of, args>, std::is_base_of>>; template - using argument_is_kwargs = any_of< - std::is_same, kwargs>, std::is_base_of>>; + using argument_is_kwargs = any_of, kwargs>, + std::is_base_of>>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); From b375ad7ff563bc6bd72e74c04c71f5901dab14c7 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Thu, 10 Oct 2024 16:11:01 +0100 Subject: [PATCH 07/21] Added if constexpr workaround This can probably be done better but at least this is a start. --- include/pybind11/cast.h | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 372aa8a80e..44446d0788 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1564,21 +1564,35 @@ struct function_call { handle init_self; }; +template +struct is_complete : std::false_type {}; + +template +struct is_complete : std::true_type {}; + +template +constexpr bool is_same_or_base_of(){ + // See PR #5396 for the discussion that led to this + if constexpr (std::is_same){ + return true; + } + if constexpr (is_complete::value){ + // Only evaluate is_base_of if Derived is complete. + // It will raise a compiler error if Derived is not complete. + return std::is_base_of::value; + } else { + return false; + } +} + /// Helper class which loads arguments for C++ functions called from Python template class argument_loader { using indices = make_index_sequence; - - // See PR #5396 for the discussion that led to using - // any_of, std::is_base_of<...>> - // instead of simply - // std::is_base_of<...> template - using argument_is_args - = any_of, args>, std::is_base_of>>; + using argument_is_args = std::conditional_t>(), std::true_type, std::false_type>; template - using argument_is_kwargs = any_of, kwargs>, - std::is_base_of>>; + using argument_is_kwargs = std::conditional_t>(), std::true_type, std::false_type>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); From 56758c7f0f03b53361e927de39fdc3ce6d6ba965 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:11:33 +0000 Subject: [PATCH 08/21] style: pre-commit fixes --- include/pybind11/cast.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 44446d0788..1d4585bb67 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1570,13 +1570,13 @@ struct is_complete : std::false_type {}; template struct is_complete : std::true_type {}; -template -constexpr bool is_same_or_base_of(){ +template +constexpr bool is_same_or_base_of() { // See PR #5396 for the discussion that led to this - if constexpr (std::is_same){ + if constexpr (std::is_same) { return true; } - if constexpr (is_complete::value){ + if constexpr (is_complete::value) { // Only evaluate is_base_of if Derived is complete. // It will raise a compiler error if Derived is not complete. return std::is_base_of::value; @@ -1590,9 +1590,13 @@ template class argument_loader { using indices = make_index_sequence; template - using argument_is_args = std::conditional_t>(), std::true_type, std::false_type>; + using argument_is_args = std::conditional_t>(), + std::true_type, + std::false_type>; template - using argument_is_kwargs = std::conditional_t>(), std::true_type, std::false_type>; + using argument_is_kwargs = std::conditional_t>(), + std::true_type, + std::false_type>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); From e55e864f0149a070fbfb30af5e97081f74f695e6 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Thu, 10 Oct 2024 16:45:26 +0100 Subject: [PATCH 09/21] Replace if constexpr with template struct if constexpr was not added until C++ 17. I think this should do the same thing as before. --- include/pybind11/cast.h | 39 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1d4585bb67..cdc43a6a66 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1564,39 +1564,26 @@ struct function_call { handle init_self; }; -template -struct is_complete : std::false_type {}; - -template -struct is_complete : std::true_type {}; - -template -constexpr bool is_same_or_base_of() { - // See PR #5396 for the discussion that led to this - if constexpr (std::is_same) { - return true; - } - if constexpr (is_complete::value) { - // Only evaluate is_base_of if Derived is complete. - // It will raise a compiler error if Derived is not complete. - return std::is_base_of::value; - } else { - return false; - } -} +// See PR #5396 for the discussion that led to this +template +struct is_same_or_base_of : std::is_same {}; + +// Only evaluate is_base_of if Derived is complete. +// It will raise a compiler error if Derived is not complete. +template +struct is_same_or_base_of : any_of< + std::is_same, + std::is_base_of +> {}; /// Helper class which loads arguments for C++ functions called from Python template class argument_loader { using indices = make_index_sequence; template - using argument_is_args = std::conditional_t>(), - std::true_type, - std::false_type>; + using argument_is_args = is_same_or_base_of>; template - using argument_is_kwargs = std::conditional_t>(), - std::true_type, - std::false_type>; + using argument_is_kwargs = is_same_or_base_of>; // Get kwargs argument position, or -1 if not present: static constexpr auto kwargs_pos = constexpr_last(); From badb28dee4bcb82aabcd96d2051669ff45ba8987 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:45:54 +0000 Subject: [PATCH 10/21] style: pre-commit fixes --- include/pybind11/cast.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index cdc43a6a66..5a92b738ea 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1565,16 +1565,14 @@ struct function_call { }; // See PR #5396 for the discussion that led to this -template +template struct is_same_or_base_of : std::is_same {}; // Only evaluate is_base_of if Derived is complete. // It will raise a compiler error if Derived is not complete. -template -struct is_same_or_base_of : any_of< - std::is_same, - std::is_base_of -> {}; +template +struct is_same_or_base_of + : any_of, std::is_base_of> {}; /// Helper class which loads arguments for C++ functions called from Python template From 64944489bd962775293afa74cf5e3f1e4e937659 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Thu, 10 Oct 2024 21:59:04 +0100 Subject: [PATCH 11/21] Made comment clearer --- include/pybind11/cast.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5a92b738ea..293640b323 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1569,7 +1569,7 @@ template struct is_same_or_base_of : std::is_same {}; // Only evaluate is_base_of if Derived is complete. -// It will raise a compiler error if Derived is not complete. +// is_base_of raises a compiler error if Derived is incomplete. template struct is_same_or_base_of : any_of, std::is_base_of> {}; From 84cd376cb2fca87f2941916535dc03f506dbfd35 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Sat, 12 Oct 2024 11:57:28 +0100 Subject: [PATCH 12/21] Added test cases --- tests/test_class.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 9001d86b19..44dea56872 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -52,8 +52,20 @@ void bind_empty0(py::module_ &m) { } } // namespace pr4220_tripped_over_this + +namespace pr5396_forward_declared_class { + class ForwardClass; + typedef class ForwardClass* ForwardClassPtr; + class Args : public py::args {}; +} + } // namespace test_class +static_assert(!py::is_same_or_base_of::value); +static_assert(!py::is_same_or_base_of::value); +static_assert(py::is_same_or_base_of::value); +static_assert(py::is_same_or_base_of::value); + TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -554,6 +566,16 @@ TEST_SUBMODULE(class_, m) { }); test_class::pr4220_tripped_over_this::bind_empty0(m); + + // Test constructing a pybind11 class around a pointer to an incomplete type. #5396 + py::class_ (m, "ForwardClassPtr"); +} + +namespace test_class { +namespace pr5396_forward_declared_class { + // Define the forward declared class after it is used. + class ForwardClass{}; +} } template From 1c845cf9d097a34b3a057c34e5cdfa1323bea903 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 10:57:53 +0000 Subject: [PATCH 13/21] style: pre-commit fixes --- tests/test_class.cpp | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 44dea56872..5ac94b6173 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -54,17 +54,22 @@ void bind_empty0(py::module_ &m) { } // namespace pr4220_tripped_over_this namespace pr5396_forward_declared_class { - class ForwardClass; - typedef class ForwardClass* ForwardClassPtr; - class Args : public py::args {}; -} +class ForwardClass; +typedef class ForwardClass *ForwardClassPtr; +class Args : public py::args {}; +} // namespace pr5396_forward_declared_class } // namespace test_class -static_assert(!py::is_same_or_base_of::value); -static_assert(!py::is_same_or_base_of::value); +static_assert( + !py::is_same_or_base_of::value); +static_assert( + !py::is_same_or_base_of::value); static_assert(py::is_same_or_base_of::value); -static_assert(py::is_same_or_base_of::value); +static_assert( + py::is_same_or_base_of::value); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); @@ -566,17 +571,17 @@ TEST_SUBMODULE(class_, m) { }); test_class::pr4220_tripped_over_this::bind_empty0(m); - + // Test constructing a pybind11 class around a pointer to an incomplete type. #5396 - py::class_ (m, "ForwardClassPtr"); + py::class_(m, "ForwardClassPtr"); } namespace test_class { namespace pr5396_forward_declared_class { - // Define the forward declared class after it is used. - class ForwardClass{}; -} -} +// Define the forward declared class after it is used. +class ForwardClass {}; +} // namespace pr5396_forward_declared_class +} // namespace test_class template class BreaksBase { From 5965a65fc1205ff57731193d81d149ff60751262 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Sat, 12 Oct 2024 12:02:26 +0100 Subject: [PATCH 14/21] Fixed is_same_or_base_of reference --- tests/test_class.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 5ac94b6173..7528519ae6 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -62,14 +62,14 @@ class Args : public py::args {}; } // namespace test_class static_assert( - !py::is_same_or_base_of::value); static_assert( - !py::is_same_or_base_of::value); -static_assert(py::is_same_or_base_of::value); +static_assert(py::detail::is_same_or_base_of::value); static_assert( - py::is_same_or_base_of::value); + py::detail::is_same_or_base_of::value); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); From add827775740ee08127324d2970c1da87fa55c97 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:02:49 +0000 Subject: [PATCH 15/21] style: pre-commit fixes --- tests/test_class.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 7528519ae6..3863ce0980 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -61,15 +61,16 @@ class Args : public py::args {}; } // namespace test_class -static_assert( - !py::detail::is_same_or_base_of::value); -static_assert( - !py::detail::is_same_or_base_of::value); +static_assert(!py::detail::is_same_or_base_of< + py::args, + test_class::pr5396_forward_declared_class::ForwardClass>::value); +static_assert(!py::detail::is_same_or_base_of< + py::args, + test_class::pr5396_forward_declared_class::ForwardClassPtr>::value); static_assert(py::detail::is_same_or_base_of::value); static_assert( - py::detail::is_same_or_base_of::value); + py::detail::is_same_or_base_of::value); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); From c4ce357d81113bdc4210196ba7b994430cd89813 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Sat, 12 Oct 2024 12:28:11 +0100 Subject: [PATCH 16/21] Added static assert messages --- tests/test_class.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 3863ce0980..a914079bde 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -63,14 +63,17 @@ class Args : public py::args {}; static_assert(!py::detail::is_same_or_base_of< py::args, - test_class::pr5396_forward_declared_class::ForwardClass>::value); + test_class::pr5396_forward_declared_class::ForwardClass>::value, + "ForwardClass is not the same or base of py::args."); static_assert(!py::detail::is_same_or_base_of< py::args, - test_class::pr5396_forward_declared_class::ForwardClassPtr>::value); -static_assert(py::detail::is_same_or_base_of::value); + test_class::pr5396_forward_declared_class::ForwardClassPtr>::value, + "ForwardClassPtr is not the same or base of py::args."); +static_assert(py::detail::is_same_or_base_of::value, "py::args is py::args."); static_assert( py::detail::is_same_or_base_of::value); + test_class::pr5396_forward_declared_class::Args>::value, + "Args is subclass of py::args."); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); From 8b9e5d18efa50a0a057486997e6820244c554bab Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:28:34 +0000 Subject: [PATCH 17/21] style: pre-commit fixes --- tests/test_class.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index a914079bde..68e3a6580d 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -62,18 +62,18 @@ class Args : public py::args {}; } // namespace test_class static_assert(!py::detail::is_same_or_base_of< - py::args, - test_class::pr5396_forward_declared_class::ForwardClass>::value, + py::args, + test_class::pr5396_forward_declared_class::ForwardClass>::value, "ForwardClass is not the same or base of py::args."); static_assert(!py::detail::is_same_or_base_of< - py::args, - test_class::pr5396_forward_declared_class::ForwardClassPtr>::value, + py::args, + test_class::pr5396_forward_declared_class::ForwardClassPtr>::value, "ForwardClassPtr is not the same or base of py::args."); static_assert(py::detail::is_same_or_base_of::value, "py::args is py::args."); static_assert( py::detail::is_same_or_base_of::value, - "Args is subclass of py::args."); + "Args is subclass of py::args."); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); }); From ccb165be5996b709eb0b6fe0b1ac1e5556788e74 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Sat, 12 Oct 2024 12:43:44 +0100 Subject: [PATCH 18/21] Replaced typedef with using --- tests/test_class.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 68e3a6580d..115bea490a 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -55,7 +55,7 @@ void bind_empty0(py::module_ &m) { namespace pr5396_forward_declared_class { class ForwardClass; -typedef class ForwardClass *ForwardClassPtr; +using ForwardClassPtr = class ForwardClass*; class Args : public py::args {}; } // namespace pr5396_forward_declared_class From 8bc767d5aec07f40ca81b8531772ebbc463ac8cb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:44:06 +0000 Subject: [PATCH 19/21] style: pre-commit fixes --- tests/test_class.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 115bea490a..8e5ddeb285 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -55,7 +55,7 @@ void bind_empty0(py::module_ &m) { namespace pr5396_forward_declared_class { class ForwardClass; -using ForwardClassPtr = class ForwardClass*; +using ForwardClassPtr = class ForwardClass *; class Args : public py::args {}; } // namespace pr5396_forward_declared_class From b7c4fe9211be892d447122e7c033bac210eeeed0 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Oct 2024 10:48:47 -0700 Subject: [PATCH 20/21] Back out `ForwardClassPtr` (to be discussed separately). Tested locally with clang-tidy. --- tests/test_class.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 8e5ddeb285..825c224fdf 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -55,7 +55,6 @@ void bind_empty0(py::module_ &m) { namespace pr5396_forward_declared_class { class ForwardClass; -using ForwardClassPtr = class ForwardClass *; class Args : public py::args {}; } // namespace pr5396_forward_declared_class @@ -65,10 +64,6 @@ static_assert(!py::detail::is_same_or_base_of< py::args, test_class::pr5396_forward_declared_class::ForwardClass>::value, "ForwardClass is not the same or base of py::args."); -static_assert(!py::detail::is_same_or_base_of< - py::args, - test_class::pr5396_forward_declared_class::ForwardClassPtr>::value, - "ForwardClassPtr is not the same or base of py::args."); static_assert(py::detail::is_same_or_base_of::value, "py::args is py::args."); static_assert( py::detail::is_same_or_base_of(m, "ForwardClassPtr"); } -namespace test_class { -namespace pr5396_forward_declared_class { -// Define the forward declared class after it is used. -class ForwardClass {}; -} // namespace pr5396_forward_declared_class -} // namespace test_class - template class BreaksBase { public: From b778cc6779fce80dcc760c74ef3b154058ccd680 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 12 Oct 2024 10:55:27 -0700 Subject: [PATCH 21/21] Shuffle new `static_assert()` and leave error messages blank (they are more distracting than helpful here). --- tests/test_class.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 825c224fdf..cb84c327a0 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -60,15 +60,15 @@ class Args : public py::args {}; } // namespace test_class -static_assert(!py::detail::is_same_or_base_of< - py::args, - test_class::pr5396_forward_declared_class::ForwardClass>::value, - "ForwardClass is not the same or base of py::args."); -static_assert(py::detail::is_same_or_base_of::value, "py::args is py::args."); +static_assert(py::detail::is_same_or_base_of::value, ""); static_assert( py::detail::is_same_or_base_of::value, - "Args is subclass of py::args."); + ""); +static_assert(!py::detail::is_same_or_base_of< + py::args, + test_class::pr5396_forward_declared_class::ForwardClass>::value, + ""); TEST_SUBMODULE(class_, m) { m.def("obj_class_name", [](py::handle obj) { return py::detail::obj_class_name(obj.ptr()); });