From ccb1afb0dd79033b202f3352ef5cc62ec73ea6d7 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 20 Jan 2022 19:42:05 -0800 Subject: [PATCH 01/13] Make smart holder type casters support void pointer capsules. --- .../detail/smart_holder_type_casters.h | 32 ++++- tests/test_class_sh_void_ptr_capsule.cpp | 131 ++++++++++++++++++ tests/test_class_sh_void_ptr_capsule.py | 34 +++++ 3 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 tests/test_class_sh_void_ptr_capsule.cpp create mode 100644 tests/test_class_sh_void_ptr_capsule.py diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 6eb5ef2773..0c1debef69 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -360,12 +361,19 @@ struct smart_holder_type_caster_load { bool load(handle src, bool convert) { static_assert(type_uses_smart_holder_type_caster::value, "Internal consistency error."); load_impl = modified_type_caster_generic_load_impl(typeid(T)); + void_ptr_capsule = try_load_as_void_ptr_capsule(src); + if (void_ptr_capsule) { + return true; + } if (!load_impl.load(src, convert)) return false; return true; } T *loaded_as_raw_ptr_unowned() const { + if (void_ptr_capsule) { + return void_ptr_capsule; + } void *void_ptr = load_impl.unowned_void_ptr_from_direct_conversion; if (void_ptr == nullptr) { if (have_holder()) { @@ -387,6 +395,10 @@ struct smart_holder_type_caster_load { } std::shared_ptr loaded_as_shared_ptr() const { + if (void_ptr_capsule) { + throw cast_error("Void pointer capsule cannot be converted to a" + " std::shared_ptr."); + } if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr) throw cast_error("Unowned pointer from direct conversion cannot be converted to a" " std::shared_ptr."); @@ -441,6 +453,10 @@ struct smart_holder_type_caster_load { template std::unique_ptr loaded_as_unique_ptr(const char *context = "loaded_as_unique_ptr") { + if (void_ptr_capsule) { + throw cast_error("Void pointer capsule cannot be converted to a" + " std::unique_ptr."); + } if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr) throw cast_error("Unowned pointer from direct conversion cannot be converted to a" " std::unique_ptr."); @@ -492,7 +508,7 @@ struct smart_holder_type_caster_load { private: modified_type_caster_generic_load_impl load_impl; - + T* void_ptr_capsule = nullptr; bool have_holder() const { return load_impl.loaded_v_h.vh != nullptr && load_impl.loaded_v_h.holder_constructed(); } @@ -526,6 +542,20 @@ struct smart_holder_type_caster_load { } return static_cast(void_ptr); } + + T *try_load_as_void_ptr_capsule(handle src) { + std::string type_name = type_id(); + std::string function_name = "as_" + std::regex_replace( + type_name, std::regex("::"), "_"); + if (hasattr(src, function_name.c_str())) { + capsule c = reinterpret_borrow(PyObject_CallMethod( + src.ptr(), function_name.c_str(), nullptr)); + if (c.check()) { + return c.get_pointer(); + } + } + return nullptr; + } }; // SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code. diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp new file mode 100644 index 0000000000..55fb1cf9d8 --- /dev/null +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -0,0 +1,131 @@ +#include "pybind11_tests.h" + +#include + +#include + +#include + +namespace pybind11_tests { +namespace class_sh_void_ptr_capsule { + +struct CapsuleBase { + CapsuleBase() = default; + virtual ~CapsuleBase() = default; + + bool capsule_generated = false; + virtual int get() const { return 100; } +}; + +struct Valid: public CapsuleBase { + int get() const override { return 101; } + + PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { + void* vptr = static_cast(this); + capsule_generated = true; + return PyCapsule_New( + vptr, "::pybind11_tests::class_sh_void_ptr_capsule::Valid", + nullptr); + } +}; + +struct NoConversion: public CapsuleBase { + int get() const override { return 102; } +}; + +struct NoCapsuleReturned: public CapsuleBase { + int get() const { return 103; } + + PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned() { + void* vptr = static_cast(this); + capsule_generated = true; + Py_XINCREF(Py_None); + return Py_None; + } +}; + +struct AsAnotherObject: public CapsuleBase { + int get() const override { return 104; } + + PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { + void* vptr = static_cast(this); + capsule_generated = true; + return PyCapsule_New( + vptr, "::pybind11_tests::class_sh_void_ptr_capsule::Valid", + nullptr); + } +}; + +int get_from_valid_capsule(const Valid* c) { + return c->get(); +} + +int get_from_shared_ptr_valid_capsule(std::shared_ptr c) { + return c->get(); +} + +int get_from_unique_ptr_valid_capsule(std::unique_ptr c) { + return c->get(); +} + +int get_from_no_conversion_capsule(const NoConversion* c) { + return c->get(); +} + +int get_from_no_capsule_returned(const NoCapsuleReturned* c) { + return c->get(); +} + +} // namespace class_sh_void_ptr_capsule +} // namespace pybind11_tests + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::CapsuleBase) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::Valid) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoConversion) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoCapsuleReturned) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::AsAnotherObject) + +TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { + using namespace pybind11_tests::class_sh_void_ptr_capsule; + + py::classh(m, "CapsuleBase") + .def(py::init<>()) + .def("get", &CapsuleBase::get) + .def_readonly("capsule_generated", &CapsuleBase::capsule_generated); + + py::classh(m, "Valid") + .def(py::init<>()) + .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", + [](CapsuleBase* self) { + Valid *obj = dynamic_cast(self); + PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); + return pybind11::reinterpret_steal(capsule); + }); + + py::classh(m, "NoConversion") + .def(py::init<>()); + + py::classh(m, "NoCapsuleReturned") + .def(py::init<>()) + .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned", + [](CapsuleBase* self) { + NoCapsuleReturned *obj = dynamic_cast(self); + PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); + return pybind11::reinterpret_steal(capsule); + }); + + py::classh(m, "AsAnotherObject") + .def(py::init<>()) + .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", + [](CapsuleBase* self) { + AsAnotherObject *obj = dynamic_cast(self); + PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); + return pybind11::reinterpret_steal(capsule); + }); + + m.def("get_from_valid_capsule", &get_from_valid_capsule); + m.def("get_from_shared_ptr_valid_capsule", &get_from_shared_ptr_valid_capsule); + m.def("get_from_unique_ptr_valid_capsule", &get_from_unique_ptr_valid_capsule); + m.def("get_from_no_conversion_capsule", &get_from_no_conversion_capsule); + m.def("get_from_no_capsule_returned", &get_from_no_capsule_returned); +} diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py new file mode 100644 index 0000000000..bb4bc36eb7 --- /dev/null +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +import pytest + +from pybind11_tests import class_sh_void_ptr_capsule as m + + +@pytest.mark.parametrize( + "ctor, caller, expected, capsule_generated", + [ + (m.Valid, m.get_from_valid_capsule, 101, True), + (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), + (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) + ], +) +def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): + obj = ctor() + assert caller(obj) == expected + assert obj.capsule_generated == capsule_generated + + +@pytest.mark.parametrize( + "ctor, caller, pointer_type, capsule_generated", + [ + (m.AsAnotherObject, m.get_from_shared_ptr_valid_capsule, "shared_ptr", True), + (m.AsAnotherObject, m.get_from_unique_ptr_valid_capsule, "unique_ptr", True), + ], +) +def test_as_void_ptr_capsule_unsupported(ctor, caller, pointer_type, capsule_generated): + obj = ctor() + with pytest.raises(RuntimeError) as excinfo: + caller(obj) + assert pointer_type in str(excinfo.value) + assert obj.capsule_generated == capsule_generated From 3e3f70c352b6ddabd07907cc69969f6547dd5d92 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 20 Jan 2022 20:06:41 -0800 Subject: [PATCH 02/13] Fix warnings --- include/pybind11/detail/smart_holder_type_casters.h | 5 +++-- tests/test_class_sh_void_ptr_capsule.cpp | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 0c1debef69..07653e1b62 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -548,9 +548,10 @@ struct smart_holder_type_caster_load { std::string function_name = "as_" + std::regex_replace( type_name, std::regex("::"), "_"); if (hasattr(src, function_name.c_str())) { - capsule c = reinterpret_borrow(PyObject_CallMethod( + object obj = reinterpret_borrow(PyObject_CallMethod( src.ptr(), function_name.c_str(), nullptr)); - if (c.check()) { + if (isinstance(obj)) { + capsule c(obj); return c.get_pointer(); } } diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index 55fb1cf9d8..ba436dc213 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -37,7 +37,6 @@ struct NoCapsuleReturned: public CapsuleBase { int get() const { return 103; } PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned() { - void* vptr = static_cast(this); capsule_generated = true; Py_XINCREF(Py_None); return Py_None; From c42912d355ede17fba648f88eb40a27544da1af6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Jan 2022 04:26:22 +0000 Subject: [PATCH 03/13] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_class_sh_void_ptr_capsule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index bb4bc36eb7..97794f17d0 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 2fffb6fe197fb01c9803f692333dc83e5c143b7d Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Fri, 21 Jan 2022 17:08:25 -0800 Subject: [PATCH 04/13] Fix checks --- .../detail/smart_holder_type_casters.h | 72 ++++++++++--------- tests/test_class_sh_void_ptr_capsule.cpp | 4 ++ 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 07653e1b62..057ac97b62 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -107,6 +107,27 @@ class modified_type_caster_generic_load_impl { return false; } + bool try_as_void_ptr_capsule(handle src) { + std::string type_name = cpptype->name(); + detail::clean_type_id(type_name); + std::string as_void_ptr_function_name = "as_" + std::regex_replace( + type_name, std::regex("::"), "_"); + if (hasattr(src, as_void_ptr_function_name.c_str())) { + auto void_ptr_capsule = reinterpret_borrow( + PyObject_CallMethod(src.ptr(), + const_cast(as_void_ptr_function_name.c_str()), + nullptr)); + if (isinstance(void_ptr_capsule)) { + unowned_void_ptr_from_void_ptr_capsule = static_cast( + PyCapsule_GetPointer( + void_ptr_capsule.ptr(), + PyCapsule_GetName(void_ptr_capsule.ptr()))); + return true; + } + } + return false; + } + PYBIND11_NOINLINE static void *local_load(PyObject *src, const type_info *ti) { std::unique_ptr loader( new modified_type_caster_generic_load_impl(ti)); @@ -162,6 +183,9 @@ class modified_type_caster_generic_load_impl { template PYBIND11_NOINLINE bool load_impl(handle src, bool convert) { if (!src) return false; + if (cpptype && try_as_void_ptr_capsule(src)) { + return true; + } if (!typeinfo) return try_load_foreign_module_local(src); auto &this_ = static_cast(*this); @@ -251,6 +275,7 @@ class modified_type_caster_generic_load_impl { const type_info *typeinfo = nullptr; const std::type_info *cpptype = nullptr; void *unowned_void_ptr_from_direct_conversion = nullptr; + void *unowned_void_ptr_from_void_ptr_capsule = nullptr; const std::type_info *loaded_v_h_cpptype = nullptr; void *(*implicit_cast)(void *) = nullptr; value_and_holder loaded_v_h; @@ -361,28 +386,24 @@ struct smart_holder_type_caster_load { bool load(handle src, bool convert) { static_assert(type_uses_smart_holder_type_caster::value, "Internal consistency error."); load_impl = modified_type_caster_generic_load_impl(typeid(T)); - void_ptr_capsule = try_load_as_void_ptr_capsule(src); - if (void_ptr_capsule) { - return true; - } if (!load_impl.load(src, convert)) return false; return true; } T *loaded_as_raw_ptr_unowned() const { - if (void_ptr_capsule) { - return void_ptr_capsule; - } - void *void_ptr = load_impl.unowned_void_ptr_from_direct_conversion; + void *void_ptr = load_impl.unowned_void_ptr_from_void_ptr_capsule; if (void_ptr == nullptr) { - if (have_holder()) { - throw_if_uninitialized_or_disowned_holder(); - void_ptr = holder().template as_raw_ptr_unowned(); - } else if (load_impl.loaded_v_h.vh != nullptr) - void_ptr = load_impl.loaded_v_h.value_ptr(); - if (void_ptr == nullptr) - return nullptr; + void_ptr = load_impl.unowned_void_ptr_from_direct_conversion; + if (void_ptr == nullptr) { + if (have_holder()) { + throw_if_uninitialized_or_disowned_holder(); + void_ptr = holder().template as_raw_ptr_unowned(); + } else if (load_impl.loaded_v_h.vh != nullptr) + void_ptr = load_impl.loaded_v_h.value_ptr(); + if (void_ptr == nullptr) + return nullptr; + } } return convert_type(void_ptr); } @@ -395,7 +416,7 @@ struct smart_holder_type_caster_load { } std::shared_ptr loaded_as_shared_ptr() const { - if (void_ptr_capsule) { + if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { throw cast_error("Void pointer capsule cannot be converted to a" " std::shared_ptr."); } @@ -453,7 +474,7 @@ struct smart_holder_type_caster_load { template std::unique_ptr loaded_as_unique_ptr(const char *context = "loaded_as_unique_ptr") { - if (void_ptr_capsule) { + if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { throw cast_error("Void pointer capsule cannot be converted to a" " std::unique_ptr."); } @@ -508,7 +529,7 @@ struct smart_holder_type_caster_load { private: modified_type_caster_generic_load_impl load_impl; - T* void_ptr_capsule = nullptr; + bool have_holder() const { return load_impl.loaded_v_h.vh != nullptr && load_impl.loaded_v_h.holder_constructed(); } @@ -542,21 +563,6 @@ struct smart_holder_type_caster_load { } return static_cast(void_ptr); } - - T *try_load_as_void_ptr_capsule(handle src) { - std::string type_name = type_id(); - std::string function_name = "as_" + std::regex_replace( - type_name, std::regex("::"), "_"); - if (hasattr(src, function_name.c_str())) { - object obj = reinterpret_borrow(PyObject_CallMethod( - src.ptr(), function_name.c_str(), nullptr)); - if (isinstance(obj)) { - capsule c(obj); - return c.get_pointer(); - } - } - return nullptr; - } }; // SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code. diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index ba436dc213..ead03a5441 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -23,6 +23,8 @@ struct Valid: public CapsuleBase { PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { void* vptr = static_cast(this); capsule_generated = true; + // We assume vptr out lives the capsule, so we use nullptr for the + // destructor. return PyCapsule_New( vptr, "::pybind11_tests::class_sh_void_ptr_capsule::Valid", nullptr); @@ -49,6 +51,8 @@ struct AsAnotherObject: public CapsuleBase { PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { void* vptr = static_cast(this); capsule_generated = true; + // We assume vptr out lives the capsule, so we use nullptr for the + // destructor. return PyCapsule_New( vptr, "::pybind11_tests::class_sh_void_ptr_capsule::Valid", nullptr); From e3b0e230bcf7ceb24509ef3d3892d4cd97283dae Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Fri, 21 Jan 2022 18:04:40 -0800 Subject: [PATCH 05/13] Fix check failures under CentOS --- include/pybind11/detail/smart_holder_type_casters.h | 11 +++++++++-- tests/test_class_sh_void_ptr_capsule.py | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 057ac97b62..141acbc5f0 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -110,8 +110,15 @@ class modified_type_caster_generic_load_impl { bool try_as_void_ptr_capsule(handle src) { std::string type_name = cpptype->name(); detail::clean_type_id(type_name); - std::string as_void_ptr_function_name = "as_" + std::regex_replace( - type_name, std::regex("::"), "_"); + + // Convert `a::b::c` to `a_b_c` + size_t pos = type_name.find("::"); + while (pos != std::string::npos) { + type_name.replace(pos, 2, "_"); + pos = type_name.find("::", pos); + } + + std::string as_void_ptr_function_name = "as_" + type_name; if (hasattr(src, as_void_ptr_function_name.c_str())) { auto void_ptr_capsule = reinterpret_borrow( PyObject_CallMethod(src.ptr(), diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index 97794f17d0..bb4bc36eb7 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 505bfba9eae5630c30835248bf81e71b3310f757 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 22 Jan 2022 02:05:13 +0000 Subject: [PATCH 06/13] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_class_sh_void_ptr_capsule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index bb4bc36eb7..97794f17d0 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 76dc820131d6b8492448d183c0e9389ee6a12df1 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Fri, 21 Jan 2022 18:45:04 -0800 Subject: [PATCH 07/13] Remove unused regex module --- include/pybind11/detail/smart_holder_type_casters.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 141acbc5f0..e371a31691 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include From c0b669f9b064c3adc57da70a86f92724f7ae6666 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 24 Jan 2022 13:22:51 -0800 Subject: [PATCH 08/13] Resolve comments --- .../pybind11/detail/smart_holder_type_casters.h | 15 ++++++--------- tests/test_class_sh_void_ptr_capsule.cpp | 5 +++-- tests/test_class_sh_void_ptr_capsule.py | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index e371a31691..5962e60ea4 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -113,21 +113,18 @@ class modified_type_caster_generic_load_impl { // Convert `a::b::c` to `a_b_c` size_t pos = type_name.find("::"); while (pos != std::string::npos) { - type_name.replace(pos, 2, "_"); + type_name.replace(pos, 2, 1, '_'); pos = type_name.find("::", pos); } std::string as_void_ptr_function_name = "as_" + type_name; if (hasattr(src, as_void_ptr_function_name.c_str())) { - auto void_ptr_capsule = reinterpret_borrow( - PyObject_CallMethod(src.ptr(), - const_cast(as_void_ptr_function_name.c_str()), - nullptr)); + auto as_void_ptr_function = function( + src.attr(as_void_ptr_function_name.c_str())); + auto void_ptr_capsule = as_void_ptr_function(); if (isinstance(void_ptr_capsule)) { - unowned_void_ptr_from_void_ptr_capsule = static_cast( - PyCapsule_GetPointer( - void_ptr_capsule.ptr(), - PyCapsule_GetName(void_ptr_capsule.ptr()))); + unowned_void_ptr_from_void_ptr_capsule = reinterpret_borrow( + void_ptr_capsule).get_pointer(); return true; } } diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index ead03a5441..d92f3a75a9 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -1,7 +1,5 @@ #include "pybind11_tests.h" -#include - #include #include @@ -101,6 +99,7 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](CapsuleBase* self) { Valid *obj = dynamic_cast(self); + assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); }); @@ -113,6 +112,7 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned", [](CapsuleBase* self) { NoCapsuleReturned *obj = dynamic_cast(self); + assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); return pybind11::reinterpret_steal(capsule); }); @@ -122,6 +122,7 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](CapsuleBase* self) { AsAnotherObject *obj = dynamic_cast(self); + assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); }); diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index 97794f17d0..bb4bc36eb7 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 6c48f5633ccb7665330a22e3299d26632671d385 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 24 Jan 2022 21:23:29 +0000 Subject: [PATCH 09/13] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_class_sh_void_ptr_capsule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index bb4bc36eb7..97794f17d0 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From dc45d1b80432e8c7e0a110b380185ae4f808877f Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 24 Jan 2022 16:27:51 -0800 Subject: [PATCH 10/13] Resolve comments --- .../detail/smart_holder_type_casters.h | 37 ++++++++------ tests/test_class_sh_void_ptr_capsule.cpp | 51 +++++++++++-------- tests/test_class_sh_void_ptr_capsule.py | 2 +- 3 files changed, 53 insertions(+), 37 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 5962e60ea4..82e7753c6b 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -37,6 +37,15 @@ struct is_smart_holder_type : std::true_type {}; inline void register_instance(instance *self, void *valptr, const type_info *tinfo); inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo); +// Replace all occurrences of a character in string. +inline void replace_all(std::string& str, std::string from, char to) { + size_t pos = str.find(from); + while (pos != std::string::npos) { + str.replace(pos, from.length(), 1, to); + pos = str.find(from, pos); + } +} + // The modified_type_caster_generic_load_impl could replace type_caster_generic::load_impl but not // vice versa. The main difference is that the original code only propagates a reference to the // held value, while the modified implementation propagates value_and_holder. @@ -111,11 +120,7 @@ class modified_type_caster_generic_load_impl { detail::clean_type_id(type_name); // Convert `a::b::c` to `a_b_c` - size_t pos = type_name.find("::"); - while (pos != std::string::npos) { - type_name.replace(pos, 2, 1, '_'); - pos = type_name.find("::", pos); - } + replace_all(type_name, "::", '_'); std::string as_void_ptr_function_name = "as_" + type_name; if (hasattr(src, as_void_ptr_function_name.c_str())) { @@ -398,15 +403,15 @@ struct smart_holder_type_caster_load { void *void_ptr = load_impl.unowned_void_ptr_from_void_ptr_capsule; if (void_ptr == nullptr) { void_ptr = load_impl.unowned_void_ptr_from_direct_conversion; - if (void_ptr == nullptr) { - if (have_holder()) { - throw_if_uninitialized_or_disowned_holder(); - void_ptr = holder().template as_raw_ptr_unowned(); - } else if (load_impl.loaded_v_h.vh != nullptr) - void_ptr = load_impl.loaded_v_h.value_ptr(); - if (void_ptr == nullptr) - return nullptr; - } + } + if (void_ptr == nullptr) { + if (have_holder()) { + throw_if_uninitialized_or_disowned_holder(); + void_ptr = holder().template as_raw_ptr_unowned(); + } else if (load_impl.loaded_v_h.vh != nullptr) + void_ptr = load_impl.loaded_v_h.value_ptr(); + if (void_ptr == nullptr) + return nullptr; } return convert_type(void_ptr); } @@ -420,7 +425,7 @@ struct smart_holder_type_caster_load { std::shared_ptr loaded_as_shared_ptr() const { if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { - throw cast_error("Void pointer capsule cannot be converted to a" + throw cast_error("Unowned pointer from a void pointer capsule cannot be converted to a" " std::shared_ptr."); } if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr) @@ -478,7 +483,7 @@ struct smart_holder_type_caster_load { template std::unique_ptr loaded_as_unique_ptr(const char *context = "loaded_as_unique_ptr") { if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { - throw cast_error("Void pointer capsule cannot be converted to a" + throw cast_error("Unowned pointer from a void pointer capsule cannot be converted to a" " std::unique_ptr."); } if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr) diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index d92f3a75a9..7f0b0a158a 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -7,19 +7,30 @@ namespace pybind11_tests { namespace class_sh_void_ptr_capsule { -struct CapsuleBase { - CapsuleBase() = default; - virtual ~CapsuleBase() = default; +// Without the helper we will run into a type_caster::load recursion. +// This is because whenever the type_caster::load is called, it checks +// whether the object defines an `as_` method that returns the void pointer +// capsule. If yes, it calls the method. But in the following testcases, those +// `as_` methods are defined with pybind11, which implicitly takes the object +// itself as the first parameter. Therefore calling those methods causes loading +// the object again, which causes infinite recursion. +// This test is unusual in the sense that the void pointer capsules are meant to +// be provided by objects wrapped with systems other than pybind11 +// (i.e. having to avoid the recursion is an artificial problem, not the norm). +// Conveniently, the helper also serves to keep track of `capsule_generated`. +struct HelperBase { + HelperBase() = default; + virtual ~HelperBase() = default; bool capsule_generated = false; virtual int get() const { return 100; } }; -struct Valid: public CapsuleBase { +struct Valid: public HelperBase { int get() const override { return 101; } PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { - void* vptr = static_cast(this); + void* vptr = dynamic_cast(this); capsule_generated = true; // We assume vptr out lives the capsule, so we use nullptr for the // destructor. @@ -29,11 +40,11 @@ struct Valid: public CapsuleBase { } }; -struct NoConversion: public CapsuleBase { +struct NoConversion: public HelperBase { int get() const override { return 102; } }; -struct NoCapsuleReturned: public CapsuleBase { +struct NoCapsuleReturned: public HelperBase { int get() const { return 103; } PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned() { @@ -43,11 +54,11 @@ struct NoCapsuleReturned: public CapsuleBase { } }; -struct AsAnotherObject: public CapsuleBase { +struct AsAnotherObject: public HelperBase { int get() const override { return 104; } PyObject* as_pybind11_tests_class_sh_void_ptr_capsule_Valid() { - void* vptr = static_cast(this); + void* vptr = dynamic_cast(this); capsule_generated = true; // We assume vptr out lives the capsule, so we use nullptr for the // destructor. @@ -80,7 +91,7 @@ int get_from_no_capsule_returned(const NoCapsuleReturned* c) { } // namespace class_sh_void_ptr_capsule } // namespace pybind11_tests -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::CapsuleBase) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::HelperBase) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::Valid) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoConversion) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoCapsuleReturned) @@ -89,38 +100,38 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::As TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { using namespace pybind11_tests::class_sh_void_ptr_capsule; - py::classh(m, "CapsuleBase") + py::classh(m, "HelperBase") .def(py::init<>()) - .def("get", &CapsuleBase::get) - .def_readonly("capsule_generated", &CapsuleBase::capsule_generated); + .def("get", &HelperBase::get) + .def_readonly("capsule_generated", &HelperBase::capsule_generated); - py::classh(m, "Valid") + py::classh(m, "Valid") .def(py::init<>()) .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", - [](CapsuleBase* self) { + [](HelperBase* self) { Valid *obj = dynamic_cast(self); assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); }); - py::classh(m, "NoConversion") + py::classh(m, "NoConversion") .def(py::init<>()); - py::classh(m, "NoCapsuleReturned") + py::classh(m, "NoCapsuleReturned") .def(py::init<>()) .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned", - [](CapsuleBase* self) { + [](HelperBase* self) { NoCapsuleReturned *obj = dynamic_cast(self); assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); return pybind11::reinterpret_steal(capsule); }); - py::classh(m, "AsAnotherObject") + py::classh(m, "AsAnotherObject") .def(py::init<>()) .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", - [](CapsuleBase* self) { + [](HelperBase* self) { AsAnotherObject *obj = dynamic_cast(self); assert(obj != nullptr); PyObject* capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index 97794f17d0..bb4bc36eb7 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 82b6363998124c8d7f410bac4eb0ccaa0955aa51 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 25 Jan 2022 00:28:27 +0000 Subject: [PATCH 11/13] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_class_sh_void_ptr_capsule.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index bb4bc36eb7..97794f17d0 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -10,7 +10,7 @@ (m.Valid, m.get_from_valid_capsule, 101, True), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), - (m.AsAnotherObject, m.get_from_valid_capsule, 104, True) + (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), ], ) def test_as_void_ptr_capsule(ctor, caller, expected, capsule_generated): From 959543912a94d376c50273594a3bbe41f81a3e4b Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 24 Jan 2022 17:20:48 -0800 Subject: [PATCH 12/13] Fix clangtidy --- include/pybind11/detail/smart_holder_type_casters.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 82e7753c6b..b8019cf242 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -38,7 +38,7 @@ inline void register_instance(instance *self, void *valptr, const type_info *tin inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo); // Replace all occurrences of a character in string. -inline void replace_all(std::string& str, std::string from, char to) { +inline void replace_all(std::string& str, const std::string& from, char to) { size_t pos = str.find(from); while (pos != std::string::npos) { str.replace(pos, from.length(), 1, to); From f571da25c3634ba0f499b4e41321e673059512d1 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Tue, 25 Jan 2022 11:06:19 -0800 Subject: [PATCH 13/13] Resolve comments --- include/pybind11/detail/smart_holder_type_casters.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index b8019cf242..c76ce9f4ab 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -122,7 +122,8 @@ class modified_type_caster_generic_load_impl { // Convert `a::b::c` to `a_b_c` replace_all(type_name, "::", '_'); - std::string as_void_ptr_function_name = "as_" + type_name; + std::string as_void_ptr_function_name("as_"); + as_void_ptr_function_name += type_name; if (hasattr(src, as_void_ptr_function_name.c_str())) { auto as_void_ptr_function = function( src.attr(as_void_ptr_function_name.c_str()));