From 835e75726266c7f4852e55c5ba47dbdfaf6f4831 Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Mon, 25 Jan 2021 10:30:27 +0000 Subject: [PATCH 01/10] When determining if a shared_ptr already exists, use a test on the weak_ptr instead of a try/catch block. --- include/pybind11/pybind11.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3bffbb28d2..6be61b714f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,14 +1505,14 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { - try { + if (!v_h.value_ptr()->weak_from_this().expired()) { auto sh = std::dynamic_pointer_cast( v_h.value_ptr()->shared_from_this()); if (sh) { new (std::addressof(v_h.holder())) holder_type(std::move(sh)); v_h.set_holder_constructed(); } - } catch (const std::bad_weak_ptr &) {} + } if (!v_h.holder_constructed() && inst->owned) { new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); From dbc2526aaf0c3899b2b0f499b98a29fd70cc50f1 Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Mon, 25 Jan 2021 10:30:27 +0000 Subject: [PATCH 02/10] When determining if a shared_ptr already exists, use a test on the weak_ptr instead of a try/catch block. --- include/pybind11/pybind11.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3bffbb28d2..6be61b714f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,14 +1505,14 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { - try { + if (!v_h.value_ptr()->weak_from_this().expired()) { auto sh = std::dynamic_pointer_cast( v_h.value_ptr()->shared_from_this()); if (sh) { new (std::addressof(v_h.holder())) holder_type(std::move(sh)); v_h.set_holder_constructed(); } - } catch (const std::bad_weak_ptr &) {} + } if (!v_h.holder_constructed() && inst->owned) { new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); From c7210a20186acb5e0bdc01b1f8b7aa007b4fe196 Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Thu, 28 Jan 2021 10:37:45 +0000 Subject: [PATCH 03/10] weak_from_this is only available in C++17 and later --- include/pybind11/pybind11.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6be61b714f..9b5228626f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,14 +1505,22 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { +#if defined(PYBIND11_CPP17) if (!v_h.value_ptr()->weak_from_this().expired()) { +#else + try { +#endif auto sh = std::dynamic_pointer_cast( v_h.value_ptr()->shared_from_this()); if (sh) { new (std::addressof(v_h.holder())) holder_type(std::move(sh)); v_h.set_holder_constructed(); } +#if defined(PYBIND11_CPP17) } +#else + } catch (const std::bad_weak_ptr &) {} +#endif if (!v_h.holder_constructed() && inst->owned) { new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); From a29d196ddfed54872118f3852b61618e6210485a Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Sat, 30 Jan 2021 05:21:20 +0000 Subject: [PATCH 04/10] Switch to use feature flag instead of C++ version flag. --- include/pybind11/pybind11.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9b5228626f..6a643bba8b 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,7 +1505,7 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { -#if defined(PYBIND11_CPP17) +#if defined(__cpp_lib_enable_shared_from_this) if (!v_h.value_ptr()->weak_from_this().expired()) { #else try { @@ -1516,7 +1516,7 @@ class class_ : public detail::generic_type { new (std::addressof(v_h.holder())) holder_type(std::move(sh)); v_h.set_holder_constructed(); } -#if defined(PYBIND11_CPP17) +#if defined(__cpp_lib_enable_shared_from_this) } #else } catch (const std::bad_weak_ptr &) {} From 6e375b6defc9c5231ea81ecc4eb1a701ab174dcd Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Sat, 30 Jan 2021 05:44:45 +0000 Subject: [PATCH 05/10] Add Microsoft-specific check. --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6a643bba8b..2aeb34c1a5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,7 +1505,7 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { -#if defined(__cpp_lib_enable_shared_from_this) +#if (__cpp_lib_enable_shared_from_this >= 201603L) && (!defined(_MSC_VER) || _MSC_VER >= 1912) if (!v_h.value_ptr()->weak_from_this().expired()) { #else try { From 8185b6f7f446e0ca99350f26bf7e551ee7ebe0e0 Mon Sep 17 00:00:00 2001 From: Edward Lockhart Date: Sat, 30 Jan 2021 05:51:10 +0000 Subject: [PATCH 06/10] Avoid undefined preprocessor macro warning treated as error. --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 2aeb34c1a5..79ce1e8354 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1505,7 +1505,7 @@ class class_ : public detail::generic_type { template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { -#if (__cpp_lib_enable_shared_from_this >= 201603L) && (!defined(_MSC_VER) || _MSC_VER >= 1912) +#if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912) if (!v_h.value_ptr()->weak_from_this().expired()) { #else try { From 651fd4184c2c7bc4bc1823eca6f8c215507547b9 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sat, 30 Jan 2021 14:11:53 +0100 Subject: [PATCH 07/10] Simplify shared_from_this in init_holder --- include/pybind11/pybind11.h | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 79ce1e8354..56ddc352e2 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1501,26 +1501,34 @@ class class_ : public detail::generic_type { } private: - /// Initialize holder object, variant 1: object derives from enable_shared_from_this template - static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, - const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { + inline static std::shared_ptr get_shared_from_this(std::enable_shared_from_this *holder_value_ptr) { #if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912) - if (!v_h.value_ptr()->weak_from_this().expired()) { + if (!holder_value_ptr->weak_from_this().expired()) + return holder_value_ptr->shared_from_this(); + else + return nullptr; #else try { -#endif - auto sh = std::dynamic_pointer_cast( - v_h.value_ptr()->shared_from_this()); - if (sh) { - new (std::addressof(v_h.holder())) holder_type(std::move(sh)); - v_h.set_holder_constructed(); - } -#if defined(__cpp_lib_enable_shared_from_this) + return holder_value_ptr->shared_from_this(); + } + catch (const std::bad_weak_ptr &) { + return nullptr; } -#else - } catch (const std::bad_weak_ptr &) {} #endif + } + + /// Initialize holder object, variant 1: object derives from enable_shared_from_this + template + static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, + const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { + + auto sh = std::dynamic_pointer_cast( + get_shared_from_this(v_h.value_ptr())); + if (sh) { + new (std::addressof(v_h.holder())) holder_type(std::move(sh)); + v_h.set_holder_constructed(); + } if (!v_h.holder_constructed() && inst->owned) { new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); From f8b140620022013950e24e18885987de8d3b64b4 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sat, 30 Jan 2021 14:12:53 +0100 Subject: [PATCH 08/10] Include in detail/common.h (~stolen~ borrowed from @bstaletic's #2816) --- include/pybind11/detail/common.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 950bd378cd..353a4aed71 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -162,6 +162,11 @@ #include #include #include +#if defined(__has_include) +# if __has_include() +# include +# endif +#endif #if PY_MAJOR_VERSION >= 3 /// Compatibility macros for various Python versions #define PYBIND11_INSTANCE_METHOD_NEW(ptr, class_) PyInstanceMethod_New(ptr) From 97a79f55dc913537052ffbae0196a6bc62e8b43b Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sat, 30 Jan 2021 18:29:41 +0100 Subject: [PATCH 09/10] Move class_::get_shared_from_this to detail::try_get_shared_from_this --- include/pybind11/detail/common.h | 21 +++++++++++++++++++++ include/pybind11/pybind11.h | 19 +------------------ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 353a4aed71..639872fd3d 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -862,5 +862,26 @@ class any_container { // Forward-declaration; see detail/class.h std::string get_fully_qualified_tp_name(PyTypeObject*); +template +inline static std::shared_ptr try_get_shared_from_this(std::enable_shared_from_this *holder_value_ptr) { +// Pre C++17, this code path exploits undefined behavior, but is known to work on many platforms. +// Use at your own risk! +// See also https://en.cppreference.com/w/cpp/memory/enable_shared_from_this, and in particular +// the `std::shared_ptr gp1 = not_so_good.getptr();` and `try`-`catch` parts of the example. +#if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912) + if (!holder_value_ptr->weak_from_this().expired()) + return holder_value_ptr->shared_from_this(); + else + return nullptr; +#else + try { + return holder_value_ptr->shared_from_this(); + } + catch (const std::bad_weak_ptr &) { + return nullptr; + } +#endif +} + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 56ddc352e2..931752aad9 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1501,30 +1501,13 @@ class class_ : public detail::generic_type { } private: - template - inline static std::shared_ptr get_shared_from_this(std::enable_shared_from_this *holder_value_ptr) { -#if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912) - if (!holder_value_ptr->weak_from_this().expired()) - return holder_value_ptr->shared_from_this(); - else - return nullptr; -#else - try { - return holder_value_ptr->shared_from_this(); - } - catch (const std::bad_weak_ptr &) { - return nullptr; - } -#endif - } - /// Initialize holder object, variant 1: object derives from enable_shared_from_this template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { auto sh = std::dynamic_pointer_cast( - get_shared_from_this(v_h.value_ptr())); + detail::try_get_shared_from_this(v_h.value_ptr())); if (sh) { new (std::addressof(v_h.holder())) holder_type(std::move(sh)); v_h.set_holder_constructed(); From 22ba74a3ccbd6f27ee4b2f45613a32c189bebb37 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Sat, 30 Jan 2021 19:15:30 +0100 Subject: [PATCH 10/10] Simplify try_get_shared_from_this by using weak_ptr::lock() --- include/pybind11/detail/common.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 639872fd3d..bf854322b2 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -869,10 +869,7 @@ inline static std::shared_ptr try_get_shared_from_this(std::enable_shared_fro // See also https://en.cppreference.com/w/cpp/memory/enable_shared_from_this, and in particular // the `std::shared_ptr gp1 = not_so_good.getptr();` and `try`-`catch` parts of the example. #if defined(__cpp_lib_enable_shared_from_this) && (!defined(_MSC_VER) || _MSC_VER >= 1912) - if (!holder_value_ptr->weak_from_this().expired()) - return holder_value_ptr->shared_from_this(); - else - return nullptr; + return holder_value_ptr->weak_from_this().lock(); #else try { return holder_value_ptr->shared_from_this();