From bf79caafd9ca90f8761acf6a484d9eead46bc4e5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 11 Jun 2024 11:32:26 -0700 Subject: [PATCH] Remove `try_as_void_ptr_capsule` feature from smart_holder branch. This manual removal was informed by reviews of these commits (newest to oldest): * d930de0bca046774acf2cd0c09b1e4ef84d8c0bb * 114c0f2a3e94b759e32dad897f25fc979ef79a43 * 1c10b097a79008c8fb0107fb61831126784f8b88 * 9d4b4dffce6677a7c9a9af37a45f4436d353f5e1 * da058a2904bb5eb1a82417bf36ab2357a1af4d3f --- .../detail/smart_holder_type_casters.h | 95 +------------ tests/CMakeLists.txt | 1 - tests/test_class_sh_void_ptr_capsule.cpp | 127 ------------------ tests/test_class_sh_void_ptr_capsule.py | 100 -------------- 4 files changed, 1 insertion(+), 322 deletions(-) delete mode 100644 tests/test_class_sh_void_ptr_capsule.cpp delete 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 169a3659d2..c8404a75ad 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -36,72 +36,6 @@ struct is_smart_holder_type : std::true_type {}; // SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code. inline void register_instance(instance *self, void *valptr, const type_info *tinfo); inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo); -extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *, PyObject *); - -// Replace all occurrences of substrings in a string. -inline void replace_all(std::string &str, const std::string &from, const std::string &to) { - if (str.empty()) { - return; - } - size_t pos = 0; - while ((pos = str.find(from, pos)) != std::string::npos) { - str.replace(pos, from.length(), to); - pos += to.length(); - } -} - -inline bool type_is_pybind11_class_(PyTypeObject *type_obj) { -#if defined(PYPY_VERSION) - auto &internals = get_internals(); - return bool(internals.registered_types_py.find(type_obj) - != internals.registered_types_py.end()); -#else - return bool(type_obj->tp_new == pybind11_object_new); -#endif -} - -inline bool is_instance_method_of_type(PyTypeObject *type_obj, PyObject *attr_name) { - PyObject *descr = _PyType_Lookup(type_obj, attr_name); - return bool((descr != nullptr) && PyInstanceMethod_Check(descr)); -} - -inline object try_get_as_capsule_method(PyObject *obj, PyObject *attr_name) { - if (PyType_Check(obj)) { - return object(); - } - PyTypeObject *type_obj = Py_TYPE(obj); - bool known_callable = false; - if (type_is_pybind11_class_(type_obj)) { - if (!is_instance_method_of_type(type_obj, attr_name)) { - return object(); - } - known_callable = true; - } - PyObject *method = PyObject_GetAttr(obj, attr_name); - if (method == nullptr) { - PyErr_Clear(); - return object(); - } - if (!known_callable && PyCallable_Check(method) == 0) { - Py_DECREF(method); - return object(); - } - return reinterpret_steal(method); -} - -inline void *try_as_void_ptr_capsule_get_pointer(handle src, const char *typeid_name) { - std::string suffix = clean_type_id(typeid_name); - replace_all(suffix, "::", "_"); // Convert `a::b::c` to `a_b_c`. - replace_all(suffix, "*", ""); - object as_capsule_method = try_get_as_capsule_method(src.ptr(), str("as_" + suffix).ptr()); - if (as_capsule_method) { - object void_ptr_capsule = as_capsule_method(); - if (isinstance(void_ptr_capsule)) { - return reinterpret_borrow(void_ptr_capsule).get_pointer(); - } - } - return nullptr; -} // 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 @@ -176,15 +110,6 @@ class modified_type_caster_generic_load_impl { return false; } - bool try_as_void_ptr_capsule(handle src) { - unowned_void_ptr_from_void_ptr_capsule - = try_as_void_ptr_capsule_get_pointer(src, cpptype->name()); - if (unowned_void_ptr_from_void_ptr_capsule != nullptr) { - 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)); @@ -337,16 +262,12 @@ class modified_type_caster_generic_load_impl { loaded_v_h = value_and_holder(); return true; } - if (convert && cpptype && try_as_void_ptr_capsule(src)) { - return true; - } return false; } 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; std::vector implicit_casts; value_and_holder loaded_v_h; @@ -499,10 +420,7 @@ struct smart_holder_type_caster_load { } T *loaded_as_raw_ptr_unowned() const { - 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; - } + void *void_ptr = load_impl.unowned_void_ptr_from_direct_conversion; if (void_ptr == nullptr) { if (have_holder()) { throw_if_uninitialized_or_disowned_holder(typeid(T)); @@ -531,13 +449,6 @@ struct smart_holder_type_caster_load { } std::shared_ptr loaded_as_shared_ptr(handle responsible_parent = nullptr) const { - if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { - if (responsible_parent) { - return make_shared_ptr_with_responsible_parent(responsible_parent); - } - 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) { if (responsible_parent) { return make_shared_ptr_with_responsible_parent(responsible_parent); @@ -601,10 +512,6 @@ 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("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) { throw cast_error("Unowned pointer from direct conversion cannot be converted to a" " std::unique_ptr."); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c5458837b7..c1fb00fc28 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -137,7 +137,6 @@ set(PYBIND11_TEST_FILES test_class_sh_unique_ptr_custom_deleter test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix - test_class_sh_void_ptr_capsule test_classh_mock test_const_name test_constants_and_functions diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp deleted file mode 100644 index db47e3c25b..0000000000 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ /dev/null @@ -1,127 +0,0 @@ -#include - -#include "pybind11_tests.h" - -#include - -namespace pybind11_tests { -namespace class_sh_void_ptr_capsule { - -struct Valid {}; - -struct NoConversion {}; - -struct NoCapsuleReturned {}; - -struct AsAnotherObject {}; - -// Create a void pointer capsule for test. The encapsulated object does not -// matter for this test case. -PyObject *create_test_void_ptr_capsule() { - static int just_to_have_something_to_point_to = 0; - return PyCapsule_New(static_cast(&just_to_have_something_to_point_to), "int", nullptr); -} - -int get_from_valid_capsule(const Valid *) { return 1; } - -int get_from_shared_ptr_valid_capsule(const std::shared_ptr &) { return 2; } - -int get_from_unique_ptr_valid_capsule(std::unique_ptr) { return 3; } - -int get_from_no_conversion_capsule(const NoConversion *) { return 4; } - -int get_from_no_capsule_returned(const NoCapsuleReturned *) { return 5; } - -// https://github.com/pybind/pybind11/issues/3788 -struct TypeWithGetattr { - TypeWithGetattr() = default; - int get_42() const { return 42; } -}; - -// https://github.com/pybind/pybind11/issues/3804 -struct Base1 { - int a1{}; -}; -struct Base2 { - int a2{}; -}; - -struct Base12 : Base1, Base2 { - int foo() const { return 0; } -}; - -struct Derived1 : Base12 { - int bar() const { return 1; } -}; - -struct Derived2 : Base12 { - int bar() const { return 2; } -}; - -struct UnspecBase { - virtual ~UnspecBase() = default; - virtual int Get() const { return 100; } -}; - -int PassUnspecBase(const UnspecBase &sb) { return sb.Get() + 30; } - -struct UnspecDerived : UnspecBase { - int Get() const override { return 200; } -}; - -} // namespace class_sh_void_ptr_capsule -} // namespace pybind11_tests - -using namespace pybind11_tests::class_sh_void_ptr_capsule; - -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Valid) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(TypeWithGetattr) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Base1) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Base2) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Base12) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Derived1) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(Derived2) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(UnspecBase) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(UnspecDerived) - -TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { - py::classh(m, "Valid"); - - 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); - m.def("create_test_void_ptr_capsule", []() { - PyObject *capsule = create_test_void_ptr_capsule(); - return pybind11::reinterpret_steal(capsule); - }); - - py::classh(m, "TypeWithGetattr") - .def(py::init<>()) - .def("get_42", &TypeWithGetattr::get_42) - .def("__getattr__", - [](TypeWithGetattr &, const std::string &key) { return "GetAttr: " + key; }); - - py::classh(m, "Base1"); - py::classh(m, "Base2"); - - py::classh(m, "Base12") - .def(py::init<>()) - .def("foo", &Base12::foo) - .def("__getattr__", - [](Base12 &, const std::string &key) { return "Base GetAttr: " + key; }); - - py::classh(m, "Derived1").def(py::init<>()).def("bar", &Derived1::bar); - - py::classh(m, "Derived2").def(py::init<>()).def("bar", &Derived2::bar); - - py::classh(m, "UnspecBase"); - m.def("PassUnspecBase", PassUnspecBase); - py::classh(m, "UnspecDerived") // UnspecBase NOT specified as base here. - .def(py::init<>()) - .def("as_pybind11_tests_class_sh_void_ptr_capsule_UnspecBase", [](UnspecDerived *self) { - return py::reinterpret_steal( - PyCapsule_New(static_cast(self), nullptr, nullptr)); - }); -} diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py deleted file mode 100644 index 7c9824d86e..0000000000 --- a/tests/test_class_sh_void_ptr_capsule.py +++ /dev/null @@ -1,100 +0,0 @@ -import pytest - -from pybind11_tests import class_sh_void_ptr_capsule as m - - -class Valid: - def __init__(self): - self.capsule_generated = False - - def as_pybind11_tests_class_sh_void_ptr_capsule_Valid(self): - self.capsule_generated = True - return m.create_test_void_ptr_capsule() - - -class NoConversion: - def __init__(self): - self.capsule_generated = False - - -class NoCapsuleReturned: - def __init__(self): - self.capsule_generated = False - - def as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned( - self, - ): - pass - - -class AsAnotherObject: - def __init__(self): - self.capsule_generated = False - - def as_pybind11_tests_class_sh_void_ptr_capsule_Valid(self): - self.capsule_generated = True - return m.create_test_void_ptr_capsule() - - -@pytest.mark.parametrize( - ("ctor", "caller", "expected"), - [ - (Valid, m.get_from_valid_capsule, 1), - (AsAnotherObject, m.get_from_valid_capsule, 1), - ], -) -def test_valid_as_void_ptr_capsule_function(ctor, caller, expected): - obj = ctor() - assert caller(obj) == expected - assert obj.capsule_generated - - -@pytest.mark.parametrize( - ("ctor", "caller"), - [ - (NoConversion, m.get_from_no_conversion_capsule), - (NoCapsuleReturned, m.get_from_no_capsule_returned), - ], -) -def test_invalid_as_void_ptr_capsule_function(ctor, caller): - obj = ctor() - with pytest.raises(TypeError): - caller(obj) - assert not obj.capsule_generated - - -@pytest.mark.parametrize( - ("ctor", "caller", "pointer_type", "capsule_generated"), - [ - (AsAnotherObject, m.get_from_shared_ptr_valid_capsule, "shared_ptr", True), - (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 - - -def test_type_with_getattr(): - obj = m.TypeWithGetattr() - assert obj.get_42() == 42 - assert obj.something == "GetAttr: something" - - -def test_multiple_inheritance_getattr(): - d1 = m.Derived1() - assert d1.foo() == 0 - assert d1.bar() == 1 - assert d1.prop1 == "Base GetAttr: prop1" - - d2 = m.Derived2() - assert d2.foo() == 0 - assert d2.bar() == 2 - assert d2.prop2 == "Base GetAttr: prop2" - - -def test_pass_unspecified_base(): - assert m.PassUnspecBase(m.UnspecDerived()) == 230