From 6d4805ced1f8fc8f503d41381853f6082f3064a3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 Jul 2024 07:34:06 +0700 Subject: [PATCH] Small cleanup/refactoring in support of PR #5213 (#5251) * Factor out detail/value_and_holder.h (from detail/type_caster_base.h) This is in support of PR #5213: * trampoline_self_life_support.h depends on value_and_holder.h * type_caster_base.h depends on trampoline_self_life_support.h * Fix a minor and inconsequential inconsistency in `copyable_holder_caster`: the correct `load_value()` return type is `void` (as defined in `type_caster_generic`) For easy future reference, this is the long-standing inconsistency: * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/detail/type_caster_base.h#L634 * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/cast.h#L797 Noticed in passing while working on PR #5213. * Add `DANGER ZONE` comment in detail/init.h, similar to a comment added on the smart_holder branch (all the way back in 2021). --- CMakeLists.txt | 1 + include/pybind11/cast.h | 4 +- include/pybind11/detail/init.h | 6 +- include/pybind11/detail/type_caster_base.h | 62 +---------------- include/pybind11/detail/value_and_holder.h | 77 ++++++++++++++++++++++ tests/extra_python_package/test_files.py | 1 + 6 files changed, 86 insertions(+), 65 deletions(-) create mode 100644 include/pybind11/detail/value_and_holder.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 3526a1a66a..6e5b8c8f31 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -154,6 +154,7 @@ set(PYBIND11_HEADERS include/pybind11/detail/internals.h include/pybind11/detail/type_caster_base.h include/pybind11/detail/typeid.h + include/pybind11/detail/value_and_holder.h include/pybind11/attr.h include/pybind11/buffer_info.h include/pybind11/cast.h diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index e41ad2abfa..0f3091f686 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -794,11 +794,11 @@ struct copyable_holder_caster : public type_caster_base { } } - bool load_value(value_and_holder &&v_h) { + void load_value(value_and_holder &&v_h) { if (v_h.holder_constructed()) { value = v_h.value_ptr(); holder = v_h.template holder(); - return true; + return; } throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " #if !defined(PYBIND11_DETAILED_ERROR_MESSAGES) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 4509bd131e..79cc930c8d 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -128,11 +128,13 @@ void construct(value_and_holder &v_h, Cpp *ptr, bool need_alias) { // the holder and destruction happens when we leave the C++ scope, and the holder // class gets to handle the destruction however it likes. v_h.value_ptr() = ptr; - v_h.set_instance_registered(true); // To prevent init_instance from registering it - v_h.type->init_instance(v_h.inst, nullptr); // Set up the holder + v_h.set_instance_registered(true); // Trick to prevent init_instance from registering it + // DANGER ZONE BEGIN: exceptions will leave v_h in an invalid state. + v_h.type->init_instance(v_h.inst, nullptr); // Set up the holder Holder temp_holder(std::move(v_h.holder>())); // Steal the holder v_h.type->dealloc(v_h); // Destroys the moved-out holder remains, resets value ptr to null v_h.set_instance_registered(false); + // DANGER ZONE END. construct_alias_from_cpp(is_alias_constructible{}, v_h, std::move(*ptr)); } else { diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index fd8c81b9ac..1b57ee83cb 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 "typeid.h" +#include "value_and_holder.h" #include #include @@ -259,67 +260,6 @@ PYBIND11_NOINLINE handle find_registered_python_instance(void *src, }); } -struct value_and_holder { - instance *inst = nullptr; - size_t index = 0u; - const detail::type_info *type = nullptr; - void **vh = nullptr; - - // Main constructor for a found value/holder: - value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) - : inst{i}, index{index}, type{type}, - vh{inst->simple_layout ? inst->simple_value_holder - : &inst->nonsimple.values_and_holders[vpos]} {} - - // Default constructor (used to signal a value-and-holder not found by get_value_and_holder()) - value_and_holder() = default; - - // Used for past-the-end iterator - explicit value_and_holder(size_t index) : index{index} {} - - template - V *&value_ptr() const { - return reinterpret_cast(vh[0]); - } - // True if this `value_and_holder` has a non-null value pointer - explicit operator bool() const { return value_ptr() != nullptr; } - - template - H &holder() const { - return reinterpret_cast(vh[1]); - } - bool holder_constructed() const { - return inst->simple_layout - ? inst->simple_holder_constructed - : (inst->nonsimple.status[index] & instance::status_holder_constructed) != 0u; - } - // NOLINTNEXTLINE(readability-make-member-function-const) - void set_holder_constructed(bool v = true) { - if (inst->simple_layout) { - inst->simple_holder_constructed = v; - } else if (v) { - inst->nonsimple.status[index] |= instance::status_holder_constructed; - } else { - inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed; - } - } - bool instance_registered() const { - return inst->simple_layout - ? inst->simple_instance_registered - : ((inst->nonsimple.status[index] & instance::status_instance_registered) != 0); - } - // NOLINTNEXTLINE(readability-make-member-function-const) - void set_instance_registered(bool v = true) { - if (inst->simple_layout) { - inst->simple_instance_registered = v; - } else if (v) { - inst->nonsimple.status[index] |= instance::status_instance_registered; - } else { - inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_instance_registered; - } - } -}; - // Container for accessing and iterating over an instance's values/holders struct values_and_holders { private: diff --git a/include/pybind11/detail/value_and_holder.h b/include/pybind11/detail/value_and_holder.h new file mode 100644 index 0000000000..ca37d70ad2 --- /dev/null +++ b/include/pybind11/detail/value_and_holder.h @@ -0,0 +1,77 @@ +// Copyright (c) 2016-2024 The Pybind Development Team. +// All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +#pragma once + +#include "common.h" + +#include +#include + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +struct value_and_holder { + instance *inst = nullptr; + size_t index = 0u; + const detail::type_info *type = nullptr; + void **vh = nullptr; + + // Main constructor for a found value/holder: + value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) + : inst{i}, index{index}, type{type}, + vh{inst->simple_layout ? inst->simple_value_holder + : &inst->nonsimple.values_and_holders[vpos]} {} + + // Default constructor (used to signal a value-and-holder not found by get_value_and_holder()) + value_and_holder() = default; + + // Used for past-the-end iterator + explicit value_and_holder(size_t index) : index{index} {} + + template + V *&value_ptr() const { + return reinterpret_cast(vh[0]); + } + // True if this `value_and_holder` has a non-null value pointer + explicit operator bool() const { return value_ptr() != nullptr; } + + template + H &holder() const { + return reinterpret_cast(vh[1]); + } + bool holder_constructed() const { + return inst->simple_layout + ? inst->simple_holder_constructed + : (inst->nonsimple.status[index] & instance::status_holder_constructed) != 0u; + } + // NOLINTNEXTLINE(readability-make-member-function-const) + void set_holder_constructed(bool v = true) { + if (inst->simple_layout) { + inst->simple_holder_constructed = v; + } else if (v) { + inst->nonsimple.status[index] |= instance::status_holder_constructed; + } else { + inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed; + } + } + bool instance_registered() const { + return inst->simple_layout + ? inst->simple_instance_registered + : ((inst->nonsimple.status[index] & instance::status_instance_registered) != 0); + } + // NOLINTNEXTLINE(readability-make-member-function-const) + void set_instance_registered(bool v = true) { + if (inst->simple_layout) { + inst->simple_instance_registered = v; + } else if (v) { + inst->nonsimple.status[index] |= instance::status_instance_registered; + } else { + inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_instance_registered; + } + } +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index 5a3f779a7d..aedbdf1c1e 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -58,6 +58,7 @@ "include/pybind11/detail/internals.h", "include/pybind11/detail/type_caster_base.h", "include/pybind11/detail/typeid.h", + "include/pybind11/detail/value_and_holder.h", } eigen_headers = {