explicit value_and_holder::xxx_value_ptr<void>, context comments for uses of xxx_value_ptr

d96d16aa · Ralf W. Grosse-Kunstleve · bd0e0965 · d96d16aa · d96d16aa · d96d16aa
Commit d96d16aa authored Nov 25, 2020 by Ralf W. Grosse-Kunstleve
Show whitespace changes
Inline Side-by-side

Showing with 29 additions and 26 deletions

include/pybind11/cast.h
+9 -6

include/pybind11/detail/class.h
+1 -1

include/pybind11/detail/init.h
+12 -12

include/pybind11/pybind11.h
+7 -7

No files found.
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -225,11 +225,11 @@ struct value_and_holder {
    // Used for past-the-end iterator
    value_and_holder(size_t index) : index{index} {}
-    template <typename V = void> V *&xxx_value_ptr() const {
+    template <typename V> V *&xxx_value_ptr() const {
        return reinterpret_cast<V *&>(vh[0]);
    }
    // True if this `value_and_holder` has a non-null value pointer
-    explicit operator bool() const { return xxx_value_ptr(); }
+    explicit operator bool() const { return xxx_value_ptr<void>(); }
    template <typename H> H &xxx_holder() const {
        return reinterpret_cast<H &>(vh[1]);
@@ -519,7 +519,7 @@ public:
        auto inst = reinterpret_steal<object>(make_new_instance(tinfo->type));
        auto wrapper = reinterpret_cast<instance *>(inst.ptr());
        wrapper->owned = false;
-        void *&valueptr = values_and_holders(wrapper).begin()->xxx_value_ptr();
+        void *&valueptr = values_and_holders(wrapper).begin()->xxx_value_ptr<void>();  // holder_type not available here.
        switch (policy) {
            case return_value_policy::automatic:
@@ -588,7 +588,7 @@ public:
    // Base methods for generic caster; there are overridden in copyable_holder_caster
    void load_value(value_and_holder &&v_h) {
-        auto *&vptr = v_h.xxx_value_ptr();
+        auto *&vptr = v_h.xxx_value_ptr<void>();  // holder_type not available here.
        // Lazy allocation for unallocated values:
        if (vptr == nullptr) {
            auto *type = v_h.type ? v_h.type : typeinfo;
@@ -1132,7 +1132,7 @@ public:
        /* Check if this is a C++ type */
        auto &bases = all_type_info((PyTypeObject *) type::handle_of(h).ptr());
        if (bases.size() == 1) { // Only allowing loading from a single-value type
-            value = values_and_holders(reinterpret_cast<instance *>(h.ptr())).begin()->xxx_value_ptr();
+            value = values_and_holders(reinterpret_cast<instance *>(h.ptr())).begin()->xxx_value_ptr<void>();  // holder_type not applicable here.
            return true;
        }
@@ -1528,8 +1528,11 @@ protected:
    bool load_value(value_and_holder &&v_h) {
        if (v_h.holder_constructed()) {
-            value = v_h.xxx_value_ptr();
+            value = v_h.xxx_value_ptr<void>();
            holder = v_h.template xxx_holder<holder_type>();
+            if (holder_helper<holder_type>::get(holder) != value) {
+                throw std::runtime_error("holder.get() != value in copyable_holder_caster::load_value");
+            }
            return true;
        } else {
            throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "

--- a/include/pybind11/detail/class.h
+++ b/include/pybind11/detail/class.h
@@ -395,7 +395,7 @@ inline void clear_instance(PyObject *self) {
            // We have to deregister before we call dealloc because, for virtual MI types, we still
            // need to be able to get the parent pointers.
-            if (v_h.instance_registered() && !deregister_instance(instance, v_h.xxx_value_ptr(), v_h.type))
+            if (v_h.instance_registered() && !deregister_instance(instance, v_h.xxx_value_ptr<void>(), v_h.type))
                pybind11_fail("pybind11_object_dealloc(): Tried to deallocate unregistered instance!");
            if (instance->owned || v_h.holder_constructed())

--- a/include/pybind11/detail/init.h
+++ b/include/pybind11/detail/init.h
@@ -70,7 +70,7 @@ inline Class *construct_or_initialize(Args &&...args) { return new Class{std::fo
 template <typename Class>
 void construct_alias_from_cpp(std::true_type /*is_alias_constructible*/,
                              value_and_holder &v_h, Cpp<Class> &&base) {
-    v_h.xxx_value_ptr() = new Alias<Class>(std::move(base));
+    v_h.xxx_value_ptr<void>() = new Alias<Class>(std::move(base));  // construct_alias_from_cpp
 }
 template <typename Class>
 [[noreturn]] void construct_alias_from_cpp(std::false_type /*!is_alias_constructible*/,
@@ -104,7 +104,7 @@ void construct(value_and_holder &v_h, Cpp<Class> *ptr, bool need_alias) {
        // it was a normal instance, then steal the holder away into a local variable; thus
        // 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.xxx_value_ptr() = ptr;
+        v_h.xxx_value_ptr<void>() = ptr;  // construct
        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
        Holder<Class> temp_holder(std::move(v_h.xxx_holder<Holder<Class>>())); // Steal the holder
@@ -114,7 +114,7 @@ void construct(value_and_holder &v_h, Cpp<Class> *ptr, bool need_alias) {
        construct_alias_from_cpp<Class>(is_alias_constructible<Class>{}, v_h, std::move(*ptr));
    } else {
        // Otherwise the type isn't inherited, so we don't need an Alias
-        v_h.xxx_value_ptr() = ptr;
+        v_h.xxx_value_ptr<void>() = ptr;  // construct
    }
 }
@@ -123,7 +123,7 @@ void construct(value_and_holder &v_h, Cpp<Class> *ptr, bool need_alias) {
 template <typename Class, enable_if_t<Class::has_alias, int> = 0>
 void construct(value_and_holder &v_h, Alias<Class> *alias_ptr, bool) {
    no_nullptr(alias_ptr);
-    v_h.xxx_value_ptr() = static_cast<Cpp<Class> *>(alias_ptr);
+    v_h.xxx_value_ptr<void>() = static_cast<Cpp<Class> *>(alias_ptr);  // construct
 }
 // Holder return: copy its pointer, and move or copy the returned holder into the new instance's
@@ -138,7 +138,7 @@ void construct(value_and_holder &v_h, Holder<Class> holder, bool need_alias) {
        throw type_error("pybind11::init(): construction failed: returned holder-wrapped instance "
                         "is not an alias instance");
-    v_h.xxx_value_ptr() = ptr;
+    v_h.xxx_value_ptr<void>() = ptr;  // construct
    v_h.type->init_instance(v_h.inst, &holder);
 }
@@ -153,7 +153,7 @@ void construct(value_and_holder &v_h, Cpp<Class> &&result, bool need_alias) {
    if (Class::has_alias && need_alias)
        construct_alias_from_cpp<Class>(is_alias_constructible<Class>{}, v_h, std::move(result));
    else
-        v_h.xxx_value_ptr() = new Cpp<Class>(std::move(result));
+        v_h.xxx_value_ptr<void>() = new Cpp<Class>(std::move(result));  // construct
 }
 // return-by-value version 2: returning a value of the alias type itself.  We move-construct an
@@ -163,7 +163,7 @@ template <typename Class>
 void construct(value_and_holder &v_h, Alias<Class> &&result, bool) {
    static_assert(std::is_move_constructible<Alias<Class>>::value,
        "pybind11::init() return-by-alias-value factory function requires a movable alias class");
-    v_h.xxx_value_ptr() = new Alias<Class>(std::move(result));
+    v_h.xxx_value_ptr<void>() = new Alias<Class>(std::move(result));  // construct
 }
 // Implementing class for py::init<...>()
@@ -172,7 +172,7 @@ struct constructor {
    template <typename Class, typename... Extra, enable_if_t<!Class::has_alias, int> = 0>
    static void execute(Class &cl, const Extra&... extra) {
        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
-            v_h.xxx_value_ptr() = construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);
+            v_h.xxx_value_ptr<void>() = construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);  // constructor::execute
        }, is_new_style_constructor(), extra...);
    }
@@ -182,9 +182,9 @@ struct constructor {
    static void execute(Class &cl, const Extra&... extra) {
        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
            if (Py_TYPE(v_h.inst) == v_h.type->type)
-                v_h.xxx_value_ptr() = construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);
+                v_h.xxx_value_ptr<void>() = construct_or_initialize<Cpp<Class>>(std::forward<Args>(args)...);  // constructor::execute
            else
-                v_h.xxx_value_ptr() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
+                v_h.xxx_value_ptr<void>() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);  // constructor::execute
        }, is_new_style_constructor(), extra...);
    }
@@ -193,7 +193,7 @@ struct constructor {
                          !std::is_constructible<Cpp<Class>, Args...>::value, int> = 0>
    static void execute(Class &cl, const Extra&... extra) {
        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
-            v_h.xxx_value_ptr() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
+            v_h.xxx_value_ptr<void>() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);  // constructor::execute
        }, is_new_style_constructor(), extra...);
    }
 };
@@ -204,7 +204,7 @@ template <typename... Args> struct alias_constructor {
              enable_if_t<Class::has_alias && std::is_constructible<Alias<Class>, Args...>::value, int> = 0>
    static void execute(Class &cl, const Extra&... extra) {
        cl.def("__init__", [](value_and_holder &v_h, Args... args) {
-            v_h.xxx_value_ptr() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);
+            v_h.xxx_value_ptr<void>() = construct_or_initialize<Alias<Class>>(std::forward<Args>(args)...);  // alias_constructor::execute
        }, is_new_style_constructor(), extra...);
    }
 };

--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1457,7 +1457,7 @@ private:
            const holder_type * /* unused */, const std::enable_shared_from_this<T> * /* dummy */) {
        try {
            auto sh = std::dynamic_pointer_cast<typename holder_type::element_type>(
-                    v_h.xxx_value_ptr<type>()->shared_from_this());
+                    v_h.xxx_value_ptr<type>()->shared_from_this());  // init_holder
            if (sh) {
                new (std::addressof(v_h.xxx_holder<holder_type>())) holder_type(std::move(sh));
                v_h.set_holder_constructed();
@@ -1465,7 +1465,7 @@ private:
        } catch (const std::bad_weak_ptr &) {}
        if (!v_h.holder_constructed() && inst->owned) {
-            new (std::addressof(v_h.xxx_holder<holder_type>())) holder_type(v_h.xxx_value_ptr<type>());
+            new (std::addressof(v_h.xxx_holder<holder_type>())) holder_type(v_h.xxx_value_ptr<type>());  // init_holder
            v_h.set_holder_constructed();
        }
    }
@@ -1487,7 +1487,7 @@ private:
            init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible<holder_type>());
            v_h.set_holder_constructed();
        } else if (inst->owned || detail::always_construct_holder<holder_type>::value) {
-            new (std::addressof(v_h.xxx_holder<holder_type>())) holder_type(v_h.xxx_value_ptr<type>());
+            new (std::addressof(v_h.xxx_holder<holder_type>())) holder_type(v_h.xxx_value_ptr<type>());  // init_holder
            v_h.set_holder_constructed();
        }
    }
@@ -1499,10 +1499,10 @@ private:
    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.xxx_value_ptr(), v_h.type);
+            register_instance(inst, v_h.xxx_value_ptr<void>(), v_h.type);  // before init_holder
            v_h.set_instance_registered();
        }
-        init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.xxx_value_ptr<type>());
+        init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.xxx_value_ptr<type>());  // calling init_holder
    }
    /// Deallocates an instance; via holder, if constructed; otherwise via operator delete.
@@ -1519,12 +1519,12 @@ private:
            v_h.set_holder_constructed(false);
        }
        else {
-            detail::call_operator_delete(v_h.xxx_value_ptr<type>(),
+            detail::call_operator_delete(v_h.xxx_value_ptr<type>(),  // !v_h.holder_constructed()
                v_h.type->type_size,
                v_h.type->type_align
            );
        }
-        v_h.xxx_value_ptr() = nullptr;
+        v_h.xxx_value_ptr<void>() = nullptr;  // class_::dealloc
    }
    static detail::function_record *get_function_record(handle h) {