From 07ed8aaab1dabe24be6b0fa6efba6b0a79511b50 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Jan 2021 11:00:36 -0800 Subject: [PATCH] Removing enable_shared_from_this stub, simplifying existing code, clang-format. Open question, with respect to the original code: https://github.com/pybind/pybind11/blob/76a160070b369f8d82b945c97924227e8b835c94/include/pybind11/pybind11.h#L1510 To me it looks like the exact situation marked as `std::shared_ptr gp1 = not_so_good.getptr();` here: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this The comment there is: `// undefined behavior (until C++17) and std::bad_weak_ptr thrown (since C++17)` Does the existing code have UB pre C++17? I'll leave handling of enable_shared_from_this for later, as the need arises. --- include/pybind11/classh.h | 46 +++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/include/pybind11/classh.h b/include/pybind11/classh.h index 96510077c..16b5242b7 100644 --- a/include/pybind11/classh.h +++ b/include/pybind11/classh.h @@ -1,10 +1,11 @@ #pragma once -#include "smart_holder_poc.h" #include "pybind11.h" +#include "smart_holder_poc.h" PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +// clang-format off template class classh : public detail::generic_type { template using is_subtype = detail::is_strict_base_of; @@ -266,38 +267,31 @@ public: } private: - template - static void init_holder(bool /*owned*/, detail::value_and_holder &/*v_h*/, - holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { - throw std::runtime_error("Not implemented: classh::init_holder enable_shared_from_this."); - } + // clang-format on + static void init_instance(detail::instance *inst, const void *holder_const_void_ptr) { + // Need for const_cast is a consequence of the type_info::init_instance type: + // void (*init_instance)(instance *, const void *); + auto holder_void_ptr = const_cast(holder_const_void_ptr); - static void init_holder(bool owned, detail::value_and_holder &v_h, - holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this) */) { - if (holder_ptr) { - new (std::addressof(v_h.holder())) holder_type( - std::move(*holder_ptr)); - } else if (owned) { - new (std::addressof(v_h.holder())) holder_type( - holder_type::from_raw_ptr_take_ownership(v_h.value_ptr())); - } - else { - new (std::addressof(v_h.holder())) holder_type( - holder_type::from_raw_ptr_unowned(v_h.value_ptr())); - } - v_h.set_holder_constructed(); - } - - static void init_instance(detail::instance *inst, const void *holder_ptr) { auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); } - // Need for const_cast is a consequence of the type_info::init_instance type: - // void (*init_instance)(instance *, const void *); - init_holder(inst->owned, v_h, static_cast(const_cast(holder_ptr)), v_h.value_ptr()); + if (holder_void_ptr) { + // Note: inst->owned ignored. + auto holder_ptr = static_cast(holder_void_ptr); + new (std::addressof(v_h.holder())) holder_type(std::move(*holder_ptr)); + } else if (inst->owned) { + new (std::addressof(v_h.holder())) + holder_type(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr())); + } else { + new (std::addressof(v_h.holder())) + holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr())); + } + v_h.set_holder_constructed(); } + // clang-format off /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. static void dealloc(detail::value_and_holder &v_h) {