From c84dacc379535dd61f7d877bf379bfc8deac99d5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 5 Jul 2024 11:55:37 -0700 Subject: [PATCH] Factor out smart_holder_value_and_holder_support.h to avoid code duplication. All unit tests pass ASAN and MSAN testing with the Google-internal toolchain. --- CMakeLists.txt | 1 + .../detail/smart_holder_type_caster_support.h | 41 +++---------- .../smart_holder_value_and_holder_support.h | 59 +++++++++++++++++++ include/pybind11/detail/type_caster_base.h | 20 ++----- tests/extra_python_package/test_files.py | 1 + 5 files changed, 74 insertions(+), 48 deletions(-) create mode 100644 include/pybind11/detail/smart_holder_value_and_holder_support.h diff --git a/CMakeLists.txt b/CMakeLists.txt index a5ae3cd46..552b94f18 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -155,6 +155,7 @@ set(PYBIND11_HEADERS include/pybind11/detail/internals.h include/pybind11/detail/smart_holder_poc.h include/pybind11/detail/smart_holder_type_caster_support.h + include/pybind11/detail/smart_holder_value_and_holder_support.h include/pybind11/detail/type_caster_base.h include/pybind11/detail/typeid.h include/pybind11/attr.h diff --git a/include/pybind11/detail/smart_holder_type_caster_support.h b/include/pybind11/detail/smart_holder_type_caster_support.h index 5e5de66d1..0355c11fa 100644 --- a/include/pybind11/detail/smart_holder_type_caster_support.h +++ b/include/pybind11/detail/smart_holder_type_caster_support.h @@ -1,11 +1,13 @@ #pragma once +// BAKEIN_WIP: IWYU cleanup #include "../gil.h" #include "../pytypes.h" #include "../trampoline_self_life_support.h" #include "common.h" #include "dynamic_raw_ptr_cast_if_possible.h" #include "internals.h" +#include "smart_holder_value_and_holder_support.h" #include "typeid.h" #include @@ -204,11 +206,16 @@ inline std::unique_ptr unique_with_deleter(T *raw_ptr, std::unique_ptr } template -struct load_helper { +struct load_helper + : smart_holder_value_and_holder_support::value_and_holder_helper { using holder_type = pybindit::memory::smart_holder; value_and_holder loaded_v_h; + load_helper() + : smart_holder_value_and_holder_support::value_and_holder_helper( + &loaded_v_h) {} + T *loaded_as_raw_ptr_unowned() const { void *void_ptr = nullptr; if (have_holder()) { @@ -369,38 +376,6 @@ struct load_helper { #endif private: - bool have_holder() const { - return loaded_v_h.vh != nullptr && loaded_v_h.holder_constructed(); - } - - holder_type &holder() const { return loaded_v_h.holder(); } - - // BAKEIN_WIP: This needs to be factored out: see type_caster_base.h - // have_holder() must be true or this function will fail. - void throw_if_uninitialized_or_disowned_holder(const char *typeid_name) const { - static const std::string missing_value_msg = "Missing value for wrapped C++ type `"; - if (!holder().is_populated) { - throw value_error(missing_value_msg + clean_type_id(typeid_name) - + "`: Python instance is uninitialized."); - } - if (!holder().has_pointee()) { - throw value_error(missing_value_msg + clean_type_id(typeid_name) - + "`: Python instance was disowned."); - } - } - - void throw_if_uninitialized_or_disowned_holder(const std::type_info &type_info) const { - throw_if_uninitialized_or_disowned_holder(type_info.name()); - } - - // have_holder() must be true or this function will fail. - void throw_if_instance_is_currently_owned_by_shared_ptr() const { - auto vptr_gd_ptr = std::get_deleter(holder().vptr); - if (vptr_gd_ptr != nullptr && !vptr_gd_ptr->released_ptr.expired()) { - throw value_error("Python instance is currently owned by a std::shared_ptr."); - } - } - T *convert_type(void *void_ptr) const { #ifdef BAKEIN_WIP // Is this needed? implicit_casts if (void_ptr != nullptr && load_impl.loaded_v_h_cpptype != nullptr diff --git a/include/pybind11/detail/smart_holder_value_and_holder_support.h b/include/pybind11/detail/smart_holder_value_and_holder_support.h new file mode 100644 index 000000000..9d949dd60 --- /dev/null +++ b/include/pybind11/detail/smart_holder_value_and_holder_support.h @@ -0,0 +1,59 @@ +#pragma once + +// BAKEIN_WIP: IWYU cleanup +#include "common.h" +#include "smart_holder_poc.h" +#include "typeid.h" + +#include +#include +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) +PYBIND11_NAMESPACE_BEGIN(smart_holder_value_and_holder_support) + +// BAKEIN_WIP: Factor out `struct value_and_holder` from type_caster_base.h, +// then this helper does not have to be templated. +template +struct value_and_holder_helper { + const VHType *loaded_v_h; + + value_and_holder_helper(const VHType *loaded_v_h) : loaded_v_h{loaded_v_h} {} + + bool have_holder() const { + return loaded_v_h->vh != nullptr && loaded_v_h->holder_constructed(); + } + + pybindit::memory::smart_holder &holder() const { + return loaded_v_h->template holder(); + } + + void throw_if_uninitialized_or_disowned_holder(const char *typeid_name) const { + static const std::string missing_value_msg = "Missing value for wrapped C++ type `"; + if (!holder().is_populated) { + throw value_error(missing_value_msg + clean_type_id(typeid_name) + + "`: Python instance is uninitialized."); + } + if (!holder().has_pointee()) { + throw value_error(missing_value_msg + clean_type_id(typeid_name) + + "`: Python instance was disowned."); + } + } + + void throw_if_uninitialized_or_disowned_holder(const std::type_info &type_info) const { + throw_if_uninitialized_or_disowned_holder(type_info.name()); + } + + // have_holder() must be true or this function will fail. + void throw_if_instance_is_currently_owned_by_shared_ptr() const { + auto vptr_gd_ptr = std::get_deleter(holder().vptr); + if (vptr_gd_ptr != nullptr && !vptr_gd_ptr->released_ptr.expired()) { + throw value_error("Python instance is currently owned by a std::shared_ptr."); + } + } +}; + +PYBIND11_NAMESPACE_END(smart_holder_value_and_holder_support) +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 24bdc53c8..edeffa8fa 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -14,6 +14,7 @@ #include "descr.h" #include "internals.h" #include "smart_holder_poc.h" +#include "smart_holder_value_and_holder_support.h" #include "typeid.h" #include @@ -634,21 +635,10 @@ public: // Base methods for generic caster; there are overridden in copyable_holder_caster void load_value(value_and_holder &&v_h) { if (typeinfo->default_holder) { - // BAKEIN_WIP: This needs to be factored out: - // throw_if_uninitialized_or_disowned_holder() - if (v_h.vh != nullptr && v_h.holder_constructed()) { - using h_t = pybindit::memory::smart_holder; - h_t &holder = v_h.holder(); - static const std::string missing_value_msg - = "Missing value for wrapped C++ type `"; - if (!holder.is_populated) { - throw value_error(missing_value_msg + clean_type_id(typeid(cpptype).name()) - + "`: Python instance is uninitialized."); - } - if (!holder.has_pointee()) { - throw value_error(missing_value_msg + clean_type_id(typeid(cpptype).name()) - + "`: Python instance was disowned."); - } + smart_holder_value_and_holder_support::value_and_holder_helper + vh_helper(&v_h); + if (vh_helper.have_holder()) { + vh_helper.throw_if_uninitialized_or_disowned_holder(typeid(cpptype)); } } auto *&vptr = v_h.value_ptr(); diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 28a4bb2f4..ea03a7586 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -61,6 +61,7 @@ detail_headers = { "include/pybind11/detail/internals.h", "include/pybind11/detail/smart_holder_poc.h", "include/pybind11/detail/smart_holder_type_caster_support.h", + "include/pybind11/detail/smart_holder_value_and_holder_support.h", "include/pybind11/detail/type_caster_base.h", "include/pybind11/detail/typeid.h", }