From 661eeb381c2a518796c1d0ca6f734615ed2d618b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 24 Jan 2021 15:27:32 -0800 Subject: [PATCH] Adding py::smart_holder support to py::class_, purging py::classh completely. --- include/pybind11/cast.h | 4 +- include/pybind11/classh.h | 318 +----------------- include/pybind11/detail/classh_type_casters.h | 36 ++ include/pybind11/pybind11.h | 18 +- tests/classh_module_local_0.cpp | 1 + tests/classh_module_local_1.cpp | 3 +- tests/classh_module_local_2.cpp | 3 +- tests/test_classh_inheritance.cpp | 11 +- tests/test_classh_wip.cpp | 12 +- tests/test_unique_ptr_member.cpp | 4 +- 10 files changed, 78 insertions(+), 332 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 72b3a87fd..65a367642 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1628,6 +1628,7 @@ using type_caster_holder = conditional_t::val copyable_holder_caster, move_only_holder_caster>; +template struct is_smart_holder { static constexpr bool value = Value; }; template struct always_construct_holder { static constexpr bool value = Value; }; /// Create a specialization for custom holder types (silently ignores std::shared_ptr) @@ -1642,7 +1643,8 @@ template struct always_construct_holder { stati // PYBIND11_DECLARE_HOLDER_TYPE holder types: template struct is_holder_type : - std::is_base_of, detail::type_caster> {}; + detail::any_of, detail::type_caster>, + detail::is_smart_holder> {}; // Specialization for always-supported unique_ptr holders: template struct is_holder_type> : std::true_type {}; diff --git a/include/pybind11/classh.h b/include/pybind11/classh.h index 15b6e8ab9..a4c5509f6 100644 --- a/include/pybind11/classh.h +++ b/include/pybind11/classh.h @@ -1,326 +1,10 @@ #pragma once #include "detail/classh_type_casters.h" -#include "pybind11.h" #include "smart_holder_poc.h" PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) -// clang-format off -template -class classh : public detail::generic_type { - template using is_subtype = detail::is_strict_base_of; - template using is_base = detail::is_strict_base_of; - // struct instead of using here to help MSVC: - template struct is_valid_class_option : - detail::any_of, is_base> {}; - -public: - using type = type_; - using type_alias = detail::exactly_one_t; - constexpr static bool has_alias = !std::is_void::value; - using holder_type = pybindit::memory::smart_holder; - - static_assert(detail::all_of...>::value, - "Unknown/invalid classh template parameters provided"); - - static_assert(!has_alias || std::is_polymorphic::value, - "Cannot use an alias class with a non-polymorphic type"); - - PYBIND11_OBJECT(classh, generic_type, PyType_Check) - - template - classh(handle scope, const char *name, const Extra &... extra) { - using namespace detail; - - // MI can only be specified via classh template options, not constructor parameters - static_assert( - none_of...>::value || // no base class arguments, or: - ( constexpr_sum(is_pyobject::value...) == 1 && // Exactly one base - constexpr_sum(is_base::value...) == 0 && // no template option bases - none_of...>::value), // no multiple_inheritance attr - "Error: multiple inheritance bases must be specified via classh template options"); - - type_record record; - record.scope = scope; - record.name = name; - record.type = &typeid(type); - record.type_size = sizeof(conditional_t); - record.type_align = alignof(conditional_t&); - record.holder_size = sizeof(holder_type); - record.init_instance = init_instance; - record.dealloc = dealloc; - record.default_holder = false; - - set_operator_new(&record); - - /* Register base classes specified via template arguments to classh, if any */ - PYBIND11_EXPAND_SIDE_EFFECTS(add_base(record)); - - /* Process optional arguments, if any */ - process_attributes::init(extra..., &record); - - generic_type::initialize(record, &modified_type_caster_generic_load_impl::local_load); - - if (has_alias) { - auto &instances = record.module_local ? registered_local_types_cpp() : get_internals().registered_types_cpp; - instances[std::type_index(typeid(type_alias))] = instances[std::type_index(typeid(type))]; - } - } - - template ::value, int> = 0> - static void add_base(detail::type_record &rec) { - rec.add_base(typeid(Base), [](void *src) -> void * { - return static_cast(reinterpret_cast(src)); - }); - } - - template ::value, int> = 0> - static void add_base(detail::type_record &) { } - - template - classh &def(const char *name_, Func&& f, const Extra&... extra) { - cpp_function cf(method_adaptor(std::forward(f)), name(name_), is_method(*this), - sibling(getattr(*this, name_, none())), extra...); - add_class_method(*this, name_, cf); - return *this; - } - - template classh & - def_static(const char *name_, Func &&f, const Extra&... extra) { - static_assert(!std::is_member_function_pointer::value, - "def_static(...) called with a non-static member function pointer"); - cpp_function cf(std::forward(f), name(name_), scope(*this), - sibling(getattr(*this, name_, none())), extra...); - attr(cf.name()) = staticmethod(cf); - return *this; - } - - template - classh &def(const detail::op_ &op, const Extra&... extra) { - op.execute(*this, extra...); - return *this; - } - - template - classh & def_cast(const detail::op_ &op, const Extra&... extra) { - op.execute_cast(*this, extra...); - return *this; - } - - template - classh &def(const detail::initimpl::constructor &init, const Extra&... extra) { - init.execute(*this, extra...); - return *this; - } - - template - classh &def(const detail::initimpl::alias_constructor &init, const Extra&... extra) { - init.execute(*this, extra...); - return *this; - } - - template - classh &def(detail::initimpl::factory &&init, const Extra&... extra) { - std::move(init).execute(*this, extra...); - return *this; - } - - template - classh &def(detail::initimpl::pickle_factory &&pf, const Extra &...extra) { - std::move(pf).execute(*this, extra...); - return *this; - } - - template - classh& def_buffer(Func &&func) { - struct capture { Func func; }; - auto *ptr = new capture { std::forward(func) }; - install_buffer_funcs([](PyObject *obj, void *ptr) -> buffer_info* { - detail::make_caster caster; - if (!caster.load(obj, false)) - return nullptr; - return new buffer_info(((capture *) ptr)->func(caster)); - }, ptr); - weakref(m_ptr, cpp_function([ptr](handle wr) { - delete ptr; - wr.dec_ref(); - })).release(); - return *this; - } - - template - classh &def_buffer(Return (Class::*func)(Args...)) { - return def_buffer([func] (type &obj) { return (obj.*func)(); }); - } - - template - classh &def_buffer(Return (Class::*func)(Args...) const) { - return def_buffer([func] (const type &obj) { return (obj.*func)(); }); - } - - template - classh &def_readwrite(const char *name, D C::*pm, const Extra&... extra) { - static_assert(std::is_same::value || std::is_base_of::value, "def_readwrite() requires a class member (or base class member)"); - cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)), - fset([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this)); - def_property(name, fget, fset, return_value_policy::reference_internal, extra...); - return *this; - } - - template - classh &def_readonly(const char *name, const D C::*pm, const Extra& ...extra) { - static_assert(std::is_same::value || std::is_base_of::value, "def_readonly() requires a class member (or base class member)"); - cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)); - def_property_readonly(name, fget, return_value_policy::reference_internal, extra...); - return *this; - } - - template - classh &def_readwrite_static(const char *name, D *pm, const Extra& ...extra) { - cpp_function fget([pm](object) -> const D &{ return *pm; }, scope(*this)), - fset([pm](object, const D &value) { *pm = value; }, scope(*this)); - def_property_static(name, fget, fset, return_value_policy::reference, extra...); - return *this; - } - - template - classh &def_readonly_static(const char *name, const D *pm, const Extra& ...extra) { - cpp_function fget([pm](object) -> const D &{ return *pm; }, scope(*this)); - def_property_readonly_static(name, fget, return_value_policy::reference, extra...); - return *this; - } - - /// Uses return_value_policy::reference_internal by default - template - classh &def_property_readonly(const char *name, const Getter &fget, const Extra& ...extra) { - return def_property_readonly(name, cpp_function(method_adaptor(fget)), - return_value_policy::reference_internal, extra...); - } - - /// Uses cpp_function's return_value_policy by default - template - classh &def_property_readonly(const char *name, const cpp_function &fget, const Extra& ...extra) { - return def_property(name, fget, nullptr, extra...); - } - - /// Uses return_value_policy::reference by default - template - classh &def_property_readonly_static(const char *name, const Getter &fget, const Extra& ...extra) { - return def_property_readonly_static(name, cpp_function(fget), return_value_policy::reference, extra...); - } - - /// Uses cpp_function's return_value_policy by default - template - classh &def_property_readonly_static(const char *name, const cpp_function &fget, const Extra& ...extra) { - return def_property_static(name, fget, nullptr, extra...); - } - - /// Uses return_value_policy::reference_internal by default - template - classh &def_property(const char *name, const Getter &fget, const Setter &fset, const Extra& ...extra) { - return def_property(name, fget, cpp_function(method_adaptor(fset)), extra...); - } - template - classh &def_property(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) { - return def_property(name, cpp_function(method_adaptor(fget)), fset, - return_value_policy::reference_internal, extra...); - } - - /// Uses cpp_function's return_value_policy by default - template - classh &def_property(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) { - return def_property_static(name, fget, fset, is_method(*this), extra...); - } - - /// Uses return_value_policy::reference by default - template - classh &def_property_static(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) { - return def_property_static(name, cpp_function(fget), fset, return_value_policy::reference, extra...); - } - - /// Uses cpp_function's return_value_policy by default - template - classh &def_property_static(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) { - static_assert( 0 == detail::constexpr_sum(std::is_base_of::value...), - "Argument annotations are not allowed for properties"); - auto rec_fget = get_function_record(fget), rec_fset = get_function_record(fset); - auto *rec_active = rec_fget; - if (rec_fget) { - char *doc_prev = rec_fget->doc; /* 'extra' field may include a property-specific documentation string */ - detail::process_attributes::init(extra..., rec_fget); - if (rec_fget->doc && rec_fget->doc != doc_prev) { - free(doc_prev); - rec_fget->doc = strdup(rec_fget->doc); - } - } - if (rec_fset) { - char *doc_prev = rec_fset->doc; - detail::process_attributes::init(extra..., rec_fset); - if (rec_fset->doc && rec_fset->doc != doc_prev) { - free(doc_prev); - rec_fset->doc = strdup(rec_fset->doc); - } - if (! rec_active) rec_active = rec_fset; - } - def_property_static_impl(name, fget, fset, rec_active); - return *this; - } - -private: - // clang-format on - static void init_instance(detail::instance *inst, const void *holder_const_void_ptr) { - // Need for const_cast is a consequence of the type_info::init_instance type: - // void (*init_instance)(instance *, const void *); - auto holder_void_ptr = const_cast(holder_const_void_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.value_ptr(), v_h.type); - v_h.set_instance_registered(); - } - if (holder_void_ptr) { - // Note: inst->owned ignored. - auto holder_ptr = static_cast(holder_void_ptr); - new (std::addressof(v_h.holder())) holder_type(std::move(*holder_ptr)); - } else if (inst->owned) { - new (std::addressof(v_h.holder())) - holder_type(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr())); - } else { - new (std::addressof(v_h.holder())) - holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr())); - } - v_h.set_holder_constructed(); - } - // clang-format off - - /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. - static void dealloc(detail::value_and_holder &v_h) { - // We could be deallocating because we are cleaning up after a Python exception. - // If so, the Python error indicator will be set. We need to clear that before - // running the destructor, in case the destructor code calls more Python. - // If we don't, the Python API will exit with an exception, and pybind11 will - // throw error_already_set from the C++ destructor which is forbidden and triggers - // std::terminate(). - error_scope scope; - if (v_h.holder_constructed()) { - v_h.holder().~holder_type(); - v_h.set_holder_constructed(false); - } - else { - detail::call_operator_delete(v_h.value_ptr(), - v_h.type->type_size, - v_h.type->type_align - ); - } - v_h.value_ptr() = nullptr; - } - - static detail::function_record *get_function_record(handle h) { - h = detail::get_function(h); - return h ? (detail::function_record *) reinterpret_borrow(PyCFunction_GET_SELF(h.ptr())) - : nullptr; - } -}; +using pybindit::memory::smart_holder; PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/classh_type_casters.h b/include/pybind11/detail/classh_type_casters.h index 99afcdffd..dd746021a 100644 --- a/include/pybind11/detail/classh_type_casters.h +++ b/include/pybind11/detail/classh_type_casters.h @@ -631,5 +631,41 @@ struct classh_type_caster> : smart_holder_type_caster_l } \ } +template <> +struct is_smart_holder + : is_smart_holder { + + static decltype(&modified_type_caster_generic_load_impl::local_load) + get_type_caster_local_load_function_ptr() { + return &modified_type_caster_generic_load_impl::local_load; + } + + template + static void init_instance_for_type(detail::instance *inst, const void *holder_const_void_ptr) { + // Need for const_cast is a consequence of the type_info::init_instance type: + // void (*init_instance)(instance *, const void *); + auto holder_void_ptr = const_cast(holder_const_void_ptr); + + auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(T))); + if (!v_h.instance_registered()) { + register_instance(inst, v_h.value_ptr(), v_h.type); + v_h.set_instance_registered(); + } + using holder_type = pybindit::memory::smart_holder; + if (holder_void_ptr) { + // Note: inst->owned ignored. + auto holder_ptr = static_cast(holder_void_ptr); + new (std::addressof(v_h.holder())) holder_type(std::move(*holder_ptr)); + } else if (inst->owned) { + new (std::addressof(v_h.holder())) + holder_type(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr())); + } else { + new (std::addressof(v_h.holder())) + holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr())); + } + v_h.set_holder_constructed(); + } +}; + } // namespace detail } // namespace pybind11 diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index a17fd6f52..df1c2d4f3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1294,7 +1294,7 @@ public: /* Process optional arguments, if any */ process_attributes::init(extra..., &record); - generic_type::initialize(record, &type_caster_generic::local_load); + generic_type_initialize(record); if (has_alias) { auto &instances = record.module_local ? registered_local_types_cpp() : get_internals().registered_types_cpp; @@ -1502,6 +1502,16 @@ public: } private: + template ::value, int> = 0> + void generic_type_initialize(const detail::type_record &record) { + generic_type::initialize(record, &detail::type_caster_generic::local_load); + } + + template ::value, int> = 0> + void generic_type_initialize(const detail::type_record &record) { + generic_type::initialize(record, detail::is_smart_holder::get_type_caster_local_load_function_ptr()); + } + /// Initialize holder object, variant 1: object derives from enable_shared_from_this template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, @@ -1546,6 +1556,7 @@ private: /// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes an /// optional pointer to an existing holder to use; if not specified and the instance is /// `.owned`, a new holder will be constructed to manage the value pointer. + template ::value, int> = 0> 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()) { @@ -1555,6 +1566,11 @@ private: init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); } + template ::value, int> = 0> + static void init_instance(detail::instance *inst, const void *holder_ptr) { + detail::is_smart_holder::template init_instance_for_type(inst, holder_ptr); + } + /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. static void dealloc(detail::value_and_holder &v_h) { // We could be deallocating because we are cleaning up after a Python exception. diff --git a/tests/classh_module_local_0.cpp b/tests/classh_module_local_0.cpp index 39c391c95..8a80711ed 100644 --- a/tests/classh_module_local_0.cpp +++ b/tests/classh_module_local_0.cpp @@ -1,4 +1,5 @@ #include +#include #include diff --git a/tests/classh_module_local_1.cpp b/tests/classh_module_local_1.cpp index 04e21cf08..3c5948a82 100644 --- a/tests/classh_module_local_1.cpp +++ b/tests/classh_module_local_1.cpp @@ -1,5 +1,6 @@ // Identical to classh_module_local_2.cpp, except 2 replaced with 1. #include +#include #include @@ -21,7 +22,7 @@ PYBIND11_MODULE(classh_module_local_1, m) { namespace py = pybind11; using namespace pybind11_tests::classh_module_local; - py::classh(m, "atyp", py::module_local()) + py::class_(m, "atyp", py::module_local()) .def(py::init([](const std::string &mtxt) { atyp obj; obj.mtxt = mtxt; diff --git a/tests/classh_module_local_2.cpp b/tests/classh_module_local_2.cpp index 596e6626d..7316f3eeb 100644 --- a/tests/classh_module_local_2.cpp +++ b/tests/classh_module_local_2.cpp @@ -1,5 +1,6 @@ // Identical to classh_module_local_1.cpp, except 1 replaced with 2. #include +#include #include @@ -21,7 +22,7 @@ PYBIND11_MODULE(classh_module_local_2, m) { namespace py = pybind11; using namespace pybind11_tests::classh_module_local; - py::classh(m, "atyp", py::module_local()) + py::class_(m, "atyp", py::module_local()) .def(py::init([](const std::string &mtxt) { atyp obj; obj.mtxt = mtxt; diff --git a/tests/test_classh_inheritance.cpp b/tests/test_classh_inheritance.cpp index 2694d051f..6f84202f6 100644 --- a/tests/test_classh_inheritance.cpp +++ b/tests/test_classh_inheritance.cpp @@ -67,8 +67,8 @@ namespace pybind11_tests { namespace classh_inheritance { TEST_SUBMODULE(classh_inheritance, m) { - py::classh(m, "base"); - py::classh(m, "drvd"); + py::class_(m, "base"); + py::class_(m, "drvd"); auto rvto = py::return_value_policy::take_ownership; @@ -82,9 +82,10 @@ TEST_SUBMODULE(classh_inheritance, m) { m.def("pass_shcp_base", pass_shcp_base); m.def("pass_shcp_drvd", pass_shcp_drvd); - py::classh(m, "base1").def(py::init<>()); // __init__ needed for Python inheritance. - py::classh(m, "base2").def(py::init<>()); - py::classh(m, "drvd2"); + // __init__ needed for Python inheritance. + py::class_(m, "base1").def(py::init<>()); + py::class_(m, "base2").def(py::init<>()); + py::class_(m, "drvd2"); m.def("rtrn_mptr_drvd2", rtrn_mptr_drvd2, rvto); m.def("rtrn_mptr_drvd2_up_cast1", rtrn_mptr_drvd2_up_cast1, rvto); diff --git a/tests/test_classh_wip.cpp b/tests/test_classh_wip.cpp index 17e5a1014..2b0a9bbcf 100644 --- a/tests/test_classh_wip.cpp +++ b/tests/test_classh_wip.cpp @@ -57,11 +57,13 @@ namespace classh_wip { TEST_SUBMODULE(classh_wip, m) { namespace py = pybind11; - py::classh(m, "atyp").def(py::init<>()).def(py::init([](const std::string &mtxt) { - atyp obj; - obj.mtxt = mtxt; - return obj; - })); + py::class_(m, "atyp") + .def(py::init<>()) + .def(py::init([](const std::string &mtxt) { + atyp obj; + obj.mtxt = mtxt; + return obj; + })); m.def("rtrn_valu_atyp", rtrn_valu_atyp); m.def("rtrn_rref_atyp", rtrn_rref_atyp); diff --git a/tests/test_unique_ptr_member.cpp b/tests/test_unique_ptr_member.cpp index 415fb636f..071241701 100644 --- a/tests/test_unique_ptr_member.cpp +++ b/tests/test_unique_ptr_member.cpp @@ -46,7 +46,9 @@ namespace pybind11_tests { namespace unique_ptr_member { TEST_SUBMODULE(unique_ptr_member, m) { - py::classh(m, "pointee").def(py::init<>()).def("get_int", &pointee::get_int); + py::class_(m, "pointee") + .def(py::init<>()) + .def("get_int", &pointee::get_int); m.def("make_unique_pointee", make_unique_pointee);