From 15de97dd7d4b5d9fccac5584356b943f3e0d1b27 Mon Sep 17 00:00:00 2001 From: Joe Jevnik Date: Thu, 9 Jul 2020 22:09:59 -0400 Subject: [PATCH 1/2] BUG: ensure interator type stays alive --- include/libpy/autoclass.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/include/libpy/autoclass.h b/include/libpy/autoclass.h index eae6992e..bb045eae 100644 --- a/include/libpy/autoclass.h +++ b/include/libpy/autoclass.h @@ -1157,13 +1157,12 @@ class autoclass_impl { iter_name += "::iterator"; // create the iterator class and put it in the cache - if (!autoclass(std::move(iter_name), Py_TPFLAGS_HAVE_GC) - .add_slot(Py_tp_iternext, static_cast(iternext)) - .add_slot(Py_tp_iter, &PyObject_SelfIter) - .template traverse<&iter::traverse>() - .type()) { - throw py::exception{}; - } + autoclass(std::move(iter_name), Py_TPFLAGS_HAVE_GC) + .add_slot(Py_tp_iternext, static_cast(iternext)) + .add_slot(Py_tp_iter, &PyObject_SelfIter) + .template traverse<&iter::traverse>() + .type() + .escape(); return [](PyObject* self) -> PyObject* { try { From fb1e2e026625151d427856899949085922f987b2 Mon Sep 17 00:00:00 2001 From: Joe Jevnik Date: Thu, 9 Jul 2020 22:38:19 -0400 Subject: [PATCH 2/2] ENH: Add constructor for automatically adding a type to a module. --- include/libpy/autoclass.h | 73 ++++++++++++++++++++++++++++++--------- tests/_test_automodule.cc | 14 ++++---- tests/test_autoclass.cc | 8 +++++ 3 files changed, 71 insertions(+), 24 deletions(-) diff --git a/include/libpy/autoclass.h b/include/libpy/autoclass.h index bb045eae..e909f31c 100644 --- a/include/libpy/autoclass.h +++ b/include/libpy/autoclass.h @@ -89,6 +89,7 @@ class autoclass_impl { py::owned_ref m_type; PyType_Spec m_spec; py::owned_ref m_py_basetype; + py::owned_ref<> m_module; /** Check if this type uses the `Py_TPFLAGS_HAVE_GC`, which requires that we implement at least `Py_tp_traverse`, and will use `PyObject_GC_New` and `PyObject_GC_Del`. @@ -153,32 +154,32 @@ class autoclass_impl { // dispatch for free function that accepts as a first argument `T` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; // dispatch for free function that accepts as a first argument `T&` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; // dispatch for free function that accepts as a first argument `const T&` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; // dispatch for a noexcept free function that accepts as a first argument `T` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; // dispatch for noexcept free function that accepts as a first argument `T&` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; // dispatch for a noexcept free function that accepts as a first argument `const T&` template - struct free_function_impl + struct free_function_impl : public free_function_base {}; template @@ -336,19 +337,42 @@ class autoclass_impl { return static_cast(std::addressof(unbox(ob))); } +private: + std::string name_in_module(py::borrowed_ref<> module, std::string_view name) { + if (!module) { + return std::string{name}; + } + + const char* const module_name = PyModule_GetName(module.get()); + if (!module_name) { + throw py::exception{}; + } + return py::util::format_string(module_name, ".", name); + } + public: - autoclass_impl(std::string name = util::type_name(), + /** Construct the type and add it to a module. + + @param module The module to add the type to. + @param name The name of the type as seen from Python. + @param extra_flags Extra flags to forward to `tp_flags` field. + @param base_type A Python type to subclass. + */ + autoclass_impl(py::borrowed_ref<> module, + std::string name = py::util::type_name(), int extra_flags = 0, py::borrowed_ref base_type = nullptr) - : m_storage(std::make_unique(dynamic_unbox, - std::move(name))), + : m_storage( + std::make_unique(dynamic_unbox, + name_in_module(module, name))), m_type(nullptr), m_spec({m_storage->strings.front().data(), static_cast(sizeof(object)), 0, flags(extra_flags, base_type), nullptr}), - m_py_basetype(py::owned_ref::xnew_reference(base_type)) { + m_py_basetype(py::owned_ref::xnew_reference(base_type)), + m_module(py::owned_ref<>::xnew_reference(module)) { if (base_type) { // Check to make sure that the static base type is not obviously // wrong. This check does not ensure that the static base type is @@ -402,9 +426,14 @@ class autoclass_impl { add_slot(Py_tp_dealloc, py_dealloc); } - // Delete the copy constructor, the intermediate string data points into - // storage that is managed by the type until `.type()` is called. - // Also, don't try to create 2 versions of the same type. + autoclass_impl(std::string name = util::type_name(), + int extra_flags = 0, + py::borrowed_ref base_type = nullptr) + : autoclass_impl(nullptr, std::move(name), extra_flags, base_type) {} + + // Delete the copy constructor, the intermediate string data points into storage + // that is managed by the type until `.type()` is called. Also, don't try to + // create 2 versions of the same type. autoclass_impl(const autoclass_impl&) = delete; autoclass_impl(autoclass_impl&&) = default; autoclass_impl& operator=(autoclass_impl&&) = default; @@ -412,8 +441,8 @@ class autoclass_impl { /** Add a `tp_traverse` field to this type. This is only allowed, but required if `extra_flags & Py_TPFLAGS_HAVE_GC`. - @tparam impl The implementation of the traverse function. This should either be an - `int(T&, visitproc, void*)` or `int (T::*)(visitproc, void*)`. + @tparam impl The implementation of the traverse function. This should either + be an `int(T&, visitproc, void*)` or `int (T::*)(visitproc, void*)`. */ template concrete& traverse() { @@ -441,8 +470,8 @@ class autoclass_impl { /** Add a `tp_clear` field to this type. This is only allowed if `extra_flags & Py_TPFLAGS_HAVE_GC`. - @tparam impl The implementation of the clear function. This should either be an - `int(T&)` or `int (T::*)()`. + @tparam impl The implementation of the clear function. This should either be + an `int(T&)` or `int (T::*)()`. */ template concrete& clear() { @@ -1437,6 +1466,16 @@ class autoclass_impl { release_type_cache.dismiss(); m_type = type; + + if (m_module) { + const char* const last_dot = std::strrchr(m_type.get()->tp_name, '.'); + if (!last_dot) { + throw py::exception(PyExc_RuntimeError, "no '.' in type name"); + } + PyObject_SetAttrString(m_module.get(), + last_dot + 1, + static_cast(type)); + } return type; } diff --git a/tests/_test_automodule.cc b/tests/_test_automodule.cc index 01384f75..6404e7c8 100644 --- a/tests/_test_automodule.cc +++ b/tests/_test_automodule.cc @@ -25,11 +25,11 @@ LIBPY_AUTOMODULE(tests, ({py::autofunction("is_42"), py::autofunction("is_true")})) (py::borrowed_ref<> m) { - py::owned_ref t = py::autoclass("_test_automodule.int_float_pair") - .new_() - .comparisons() - .def("first") - .def("second") - .type(); - return PyObject_SetAttrString(m.get(), "int_float_pair", static_cast(t)); + py::autoclass(m, "int_float_pair") + .new_() + .comparisons() + .def("first") + .def("second") + .type(); + return false; } diff --git a/tests/test_autoclass.cc b/tests/test_autoclass.cc index e3b85752..21f1d569 100644 --- a/tests/test_autoclass.cc +++ b/tests/test_autoclass.cc @@ -16,8 +16,10 @@ namespace test_autoclass { using namespace std::literals; class autoclass : public with_python_interpreter { +protected: std::size_t m_cache_start_size; +private: void SetUp() override { // Ensure no types are hanging out before we check the cache size. gc_collect(); @@ -969,6 +971,9 @@ TEST_F(autoclass, iter) { int unboxed = py::from_object(PySequence_Fast_GET_ITEM(fast_seq.get(), ix)); EXPECT_EQ(unboxed, ix); } + + // HACK: `iter()` currently leaks the type + ++m_cache_start_size; } TEST_F(autoclass, iter_throws) { @@ -1010,6 +1015,9 @@ TEST_F(autoclass, iter_throws) { EXPECT_FALSE(fast_seq); expect_pyerr_type_and_message(PyExc_RuntimeError, "a C++ exception was raised: ayy"); PyErr_Clear(); + + // HACK: `iter()` currently leaks the type + ++m_cache_start_size; } #endif // LIBPY_AUTOCLASS_UNSAFE_API