From 0c5cc031ee4f64bbf60305051da8f45971577c3f Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 9 Oct 2020 16:46:11 +0200 Subject: [PATCH] feat: deprecate public constructors of module_ class (#2552) * Deprecated public constructors of module * Turn documentation comment of module_::add_object into valid doxygen documentation * Move definition of PYBIND11_DETAIL_MODULE_STATIC_DEF and PYBIND11_DETAIL_MODULE_CREATE macros up * Move detail::create_top_level_module to module_::create_extension_module, and unify Python 2 and 3 signature again * Throw error_already_set if module creation fails in module_::create_extension_module * Mention module_::create_extension_module in deprecation warning message of module_::module_ --- docs/Doxyfile | 3 +- include/pybind11/detail/common.h | 23 +++------- include/pybind11/embed.h | 44 +++++++++--------- include/pybind11/pybind11.h | 77 ++++++++++++++++---------------- tests/test_modules.cpp | 3 +- 5 files changed, 74 insertions(+), 76 deletions(-) diff --git a/docs/Doxyfile b/docs/Doxyfile index 24ece0d8d..c8562952e 100644 --- a/docs/Doxyfile +++ b/docs/Doxyfile @@ -19,4 +19,5 @@ QUIET = YES WARNINGS = YES WARN_IF_UNDOCUMENTED = NO PREDEFINED = DOXYGEN_SHOULD_SKIP_THIS \ - PY_MAJOR_VERSION=3 + PY_MAJOR_VERSION=3 \ + PYBIND11_NOINLINE diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index a48de0439..5dcd348fd 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -309,32 +309,23 @@ extern "C" { }); } \endrst */ -#if PY_MAJOR_VERSION >= 3 -#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \ - static PyModuleDef PYBIND11_CONCAT(pybind11_module_def_, name); -#define PYBIND11_DETAIL_MODULE_CREATE(name) \ - auto m = pybind11::detail::create_top_level_module( \ - PYBIND11_TOSTRING(name), nullptr, \ - &PYBIND11_CONCAT(pybind11_module_def_, name)); -#else -#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) -#define PYBIND11_DETAIL_MODULE_CREATE(name) \ - auto m = pybind11::module_(PYBIND11_TOSTRING(name)); -#endif #define PYBIND11_MODULE(name, variable) \ - PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \ + static ::pybind11::module_::module_def \ + PYBIND11_CONCAT(pybind11_module_def_, name); \ PYBIND11_MAYBE_UNUSED \ - static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \ + static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ PYBIND11_PLUGIN_IMPL(name) { \ PYBIND11_CHECK_PYTHON_VERSION \ PYBIND11_ENSURE_INTERNALS_READY \ - PYBIND11_DETAIL_MODULE_CREATE(name) \ + auto m = ::pybind11::module_::create_extension_module( \ + PYBIND11_TOSTRING(name), nullptr, \ + &PYBIND11_CONCAT(pybind11_module_def_, name)); \ try { \ PYBIND11_CONCAT(pybind11_init_, name)(m); \ return m.ptr(); \ } PYBIND11_CATCH_INIT_EXCEPTIONS \ } \ - void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable) + void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable) PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index 2d94e1700..aebf6b23c 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -45,26 +45,30 @@ }); } \endrst */ -#define PYBIND11_EMBEDDED_MODULE(name, variable) \ - static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \ - static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ - auto m = pybind11::module_(PYBIND11_TOSTRING(name)); \ - try { \ - PYBIND11_CONCAT(pybind11_init_, name)(m); \ - return m.ptr(); \ - } catch (pybind11::error_already_set &e) { \ - PyErr_SetString(PyExc_ImportError, e.what()); \ - return nullptr; \ - } catch (const std::exception &e) { \ - PyErr_SetString(PyExc_ImportError, e.what()); \ - return nullptr; \ - } \ - } \ - PYBIND11_EMBEDDED_MODULE_IMPL(name) \ - pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \ - (PYBIND11_TOSTRING(name), \ - PYBIND11_CONCAT(pybind11_init_impl_, name)); \ - void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable) +#define PYBIND11_EMBEDDED_MODULE(name, variable) \ + static ::pybind11::module_::module_def \ + PYBIND11_CONCAT(pybind11_module_def_, name); \ + static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \ + static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \ + auto m = ::pybind11::module_::create_extension_module( \ + PYBIND11_TOSTRING(name), nullptr, \ + &PYBIND11_CONCAT(pybind11_module_def_, name)); \ + try { \ + PYBIND11_CONCAT(pybind11_init_, name)(m); \ + return m.ptr(); \ + } catch (::pybind11::error_already_set &e) { \ + PyErr_SetString(PyExc_ImportError, e.what()); \ + return nullptr; \ + } catch (const std::exception &e) { \ + PyErr_SetString(PyExc_ImportError, e.what()); \ + return nullptr; \ + } \ + } \ + PYBIND11_EMBEDDED_MODULE_IMPL(name) \ + ::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \ + (PYBIND11_TOSTRING(name), \ + PYBIND11_CONCAT(pybind11_init_impl_, name)); \ + void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable) PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 76c53bff9..7cf88e8f5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -868,32 +868,20 @@ protected: } }; - -#if PY_MAJOR_VERSION >= 3 -class module_; - -PYBIND11_NAMESPACE_BEGIN(detail) -inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def); -PYBIND11_NAMESPACE_END(detail) -#endif - /// Wrapper for Python extension modules class module_ : public object { public: PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check) /// Create a new top-level Python module with the given name and docstring - explicit module_(const char *name, const char *doc = nullptr) + PYBIND11_DEPRECATED("Use PYBIND11_MODULE or module_::create_extension_module instead") + explicit module_(const char *name, const char *doc = nullptr) { #if PY_MAJOR_VERSION >= 3 - : module_(name, doc, new PyModuleDef()) {} + *this = create_extension_module(name, doc, new PyModuleDef()); #else - { - m_ptr = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr); - if (m_ptr == nullptr) - pybind11_fail("Internal error in module_::module_()"); - inc_ref(); - } + *this = create_extension_module(name, doc, nullptr); #endif + } /** \rst Create Python binding for a new function within the module scope. ``Func`` @@ -946,11 +934,13 @@ public: *this = reinterpret_steal(obj); } - // Adds an object to the module using the given name. Throws if an object with the given name - // already exists. - // - // overwrite should almost always be false: attempting to overwrite objects that pybind11 has - // established will, in most cases, break things. + /** \rst + Adds an object to the module using the given name. Throws if an object with the given name + already exists. + + ``overwrite`` should almost always be false: attempting to overwrite objects that pybind11 has + established will, in most cases, break things. + \endrst */ PYBIND11_NOINLINE void add_object(const char *name, handle obj, bool overwrite = false) { if (!overwrite && hasattr(*this, name)) pybind11_fail("Error during initialization: multiple incompatible definitions with name \"" + @@ -959,11 +949,21 @@ public: PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */); } -private: #if PY_MAJOR_VERSION >= 3 - friend module_ detail::create_top_level_module(const char *, const char *, PyModuleDef *); + using module_def = PyModuleDef; +#else + struct module_def {}; +#endif - explicit module_(const char *name, const char *doc, PyModuleDef *def) { + /** \rst + Create a new top-level module that can be used as the main module of a C extension. + + For Python 3, ``def`` should point to a staticly allocated module_def. + For Python 2, ``def`` can be a nullptr and is completely ignored. + \endrst */ + static module_ create_extension_module(const char *name, const char *doc, module_def *def) { +#if PY_MAJOR_VERSION >= 3 + // module_def is PyModuleDef def = new (def) PyModuleDef { // Placement new (not an allocation). /* m_base */ PyModuleDef_HEAD_INIT, /* m_name */ name, @@ -975,12 +975,21 @@ private: /* m_clear */ nullptr, /* m_free */ nullptr }; - m_ptr = PyModule_Create(def); - if (m_ptr == nullptr) - pybind11_fail("Internal error in module_::module_()"); - inc_ref(); - } + auto m = PyModule_Create(def); +#else + // Ignore module_def *def; only necessary for Python 3 + (void) def; + auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr); #endif + if (m == nullptr) { + if (PyErr_Occurred()) + throw error_already_set(); + pybind11_fail("Internal error in module_::create_extension_module()"); + } + // TODO: Sould be reinterpret_steal for Python 3, but Python also steals it again when returned from PyInit_... + // For Python 2, reinterpret_borrow is correct. + return reinterpret_borrow(m); + } }; // When inside a namespace (or anywhere as long as it's not the first item on a line), @@ -988,14 +997,6 @@ private: // simplicity, if someone wants to use py::module for example, that is perfectly safe. using module = module_; -#if PY_MAJOR_VERSION >= 3 -PYBIND11_NAMESPACE_BEGIN(detail) -inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) { - return module_(name, doc, def); -} -PYBIND11_NAMESPACE_END(detail) -#endif - /// \ingroup python_builtins /// Return a dictionary representing the global variables in the current execution frame, /// or ``__main__.__dict__`` if there is no frame (usually when the interpreter is embedded). diff --git a/tests/test_modules.cpp b/tests/test_modules.cpp index 897cf89c4..67387e809 100644 --- a/tests/test_modules.cpp +++ b/tests/test_modules.cpp @@ -62,7 +62,8 @@ TEST_SUBMODULE(modules, m) { class Dupe3 { }; class DupeException { }; - auto dm = py::module_("dummy"); + // Go ahead and leak, until we have a non-leaking py::module_ constructor + auto dm = py::module_::create_extension_module("dummy", nullptr, new py::module_::module_def); auto failures = py::list(); py::class_(dm, "Dupe1");