From e3cb2a674a3a8131717c2eb89f813698b4546fdb Mon Sep 17 00:00:00 2001 From: Khachajantc Michael Date: Wed, 20 Jun 2018 18:33:50 +0300 Subject: [PATCH] Use std::addressof to obtain holder address instead of operator& --- include/pybind11/cast.h | 4 +-- include/pybind11/pybind11.h | 10 +++--- tests/test_smart_ptr.cpp | 64 +++++++++++++++++++++++++++++++++++++ tests/test_smart_ptr.py | 44 +++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index b65d961f3..ce4dcd38a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1423,7 +1423,7 @@ public: explicit operator type*() { return this->value; } explicit operator type&() { return *(this->value); } - explicit operator holder_type*() { return &holder; } + explicit operator holder_type*() { return std::addressof(holder); } // Workaround for Intel compiler bug // see pybind11 issue 94 @@ -1493,7 +1493,7 @@ struct move_only_holder_caster { static handle cast(holder_type &&src, return_value_policy, handle) { auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + return type_caster_base::cast_holder(ptr, std::addressof(src)); } static constexpr auto name = type_caster_base::name; }; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 9c7f3688c..504e0a9bc 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1271,25 +1271,25 @@ private: auto sh = std::dynamic_pointer_cast( v_h.value_ptr()->shared_from_this()); if (sh) { - new (&v_h.holder()) holder_type(std::move(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 (&v_h.holder()) holder_type(v_h.value_ptr()); + new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); v_h.set_holder_constructed(); } } static void init_holder_from_existing(const detail::value_and_holder &v_h, const holder_type *holder_ptr, std::true_type /*is_copy_constructible*/) { - new (&v_h.holder()) holder_type(*reinterpret_cast(holder_ptr)); + new (std::addressof(v_h.holder())) holder_type(*reinterpret_cast(holder_ptr)); } static void init_holder_from_existing(const detail::value_and_holder &v_h, const holder_type *holder_ptr, std::false_type /*is_copy_constructible*/) { - new (&v_h.holder()) holder_type(std::move(*const_cast(holder_ptr))); + new (std::addressof(v_h.holder())) holder_type(std::move(*const_cast(holder_ptr))); } /// Initialize holder object, variant 2: try to construct from existing holder object, if possible @@ -1299,7 +1299,7 @@ private: init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible()); v_h.set_holder_constructed(); } else if (inst->owned || detail::always_construct_holder::value) { - new (&v_h.holder()) holder_type(v_h.value_ptr()); + new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); v_h.set_holder_constructed(); } } diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 61066f766..098b1829b 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -55,6 +55,35 @@ public: }; PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr); +// Simple custom holder that works like shared_ptr and has operator& overload +// To obtain address of an instance of this holder pybind should use std::addressof +// Attempt to get address via operator& may leads to segmentation fault +template +class shared_ptr_with_addressof_operator { + std::shared_ptr impl; +public: + shared_ptr_with_addressof_operator( ) = default; + shared_ptr_with_addressof_operator(T* p) : impl(p) { } + T* get() const { return impl.get(); } + T** operator&() { throw std::logic_error("Call of overloaded operator& is not expected"); } +}; +PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator); + +// Simple custom holder that works like unique_ptr and has operator& overload +// To obtain address of an instance of this holder pybind should use std::addressof +// Attempt to get address via operator& may leads to segmentation fault +template +class unique_ptr_with_addressof_operator { + std::unique_ptr impl; +public: + unique_ptr_with_addressof_operator() = default; + unique_ptr_with_addressof_operator(T* p) : impl(p) { } + T* get() const { return impl.get(); } + T* release_ptr() { return impl.release(); } + T** operator&() { throw std::logic_error("Call of overloaded operator& is not expected"); } +}; +PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator); + TEST_SUBMODULE(smart_ptr, m) { @@ -238,6 +267,41 @@ TEST_SUBMODULE(smart_ptr, m) { py::class_>(m, "TypeWithMoveOnlyHolder") .def_static("make", []() { return custom_unique_ptr(new C); }); + // test_holder_with_addressof_operator + struct TypeForHolderWithAddressOf { + TypeForHolderWithAddressOf() { print_created(this); } + TypeForHolderWithAddressOf(const TypeForHolderWithAddressOf &) { print_copy_created(this); } + TypeForHolderWithAddressOf(TypeForHolderWithAddressOf &&) { print_move_created(this); } + ~TypeForHolderWithAddressOf() { print_destroyed(this); } + std::string toString() const { + return "TypeForHolderWithAddressOf[" + std::to_string(value) + "]"; + } + int value = 42; + }; + using HolderWithAddressOf = shared_ptr_with_addressof_operator; + py::class_(m, "TypeForHolderWithAddressOf") + .def_static("make", []() { return HolderWithAddressOf(new TypeForHolderWithAddressOf); }) + .def("get", [](const HolderWithAddressOf &self) { return self.get(); }) + .def("print_object_1", [](const TypeForHolderWithAddressOf *obj) { py::print(obj->toString()); }) + .def("print_object_2", [](HolderWithAddressOf obj) { py::print(obj.get()->toString()); }) + .def("print_object_3", [](const HolderWithAddressOf &obj) { py::print(obj.get()->toString()); }) + .def("print_object_4", [](const HolderWithAddressOf *obj) { py::print((*obj).get()->toString()); }); + + // test_move_only_holder_with_addressof_operator + struct TypeForMoveOnlyHolderWithAddressOf { + TypeForMoveOnlyHolderWithAddressOf(int value) : value{value} { print_created(this); } + ~TypeForMoveOnlyHolderWithAddressOf() { print_destroyed(this); } + std::string toString() const { + return "MoveOnlyHolderWithAddressOf[" + std::to_string(value) + "]"; + } + int value; + }; + using MoveOnlyHolderWithAddressOf = unique_ptr_with_addressof_operator; + py::class_(m, "TypeForMoveOnlyHolderWithAddressOf") + .def_static("make", []() { return MoveOnlyHolderWithAddressOf(new TypeForMoveOnlyHolderWithAddressOf(0)); }) + .def_readwrite("value", &TypeForMoveOnlyHolderWithAddressOf::value) + .def("print_object", [](const TypeForMoveOnlyHolderWithAddressOf *obj) { py::print(obj->toString()); }); + // test_smart_ptr_from_default struct HeldByDefaultHolder { }; py::class_(m, "HeldByDefaultHolder") diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 4dfe0036f..60f48b398 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -203,6 +203,50 @@ def test_move_only_holder(): assert stats.alive() == 0 +def test_holder_with_addressof_operator(): + # this test must not throw exception from c++ + a = m.TypeForHolderWithAddressOf.make() + a.print_object_1() + a.print_object_2() + a.print_object_3() + a.print_object_4() + + stats = ConstructorStats.get(m.TypeForHolderWithAddressOf) + assert stats.alive() == 1 + + np = m.TypeForHolderWithAddressOf.make() + assert stats.alive() == 2 + del a + assert stats.alive() == 1 + del np + assert stats.alive() == 0 + + b = m.TypeForHolderWithAddressOf.make() + c = b + assert b.get() is c.get() + assert stats.alive() == 1 + + del b + assert stats.alive() == 1 + + del c + assert stats.alive() == 0 + + +def test_move_only_holder_with_addressof_operator(): + a = m.TypeForMoveOnlyHolderWithAddressOf.make() + a.print_object() + + stats = ConstructorStats.get(m.TypeForMoveOnlyHolderWithAddressOf) + assert stats.alive() == 1 + + a.value = 42 + assert a.value == 42 + + del a + assert stats.alive() == 0 + + def test_smart_ptr_from_default(): instance = m.HeldByDefaultHolder() with pytest.raises(RuntimeError) as excinfo: