Move detail::create_top_level_module to module_::create_extension_module, and unify Python 2 and 3 signature again

Author: YannickJadoul

include/pybind11/detail/common.h

@@ -261,20 +261,6 @@ extern "C" {
             return nullptr;                                                    \
         }                                                                      \
 
-#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::detail::create_top_level_module(                  \
-            PYBIND11_TOSTRING(name), nullptr);
-#endif
-
 /** \rst
     ***Deprecated in favor of PYBIND11_MODULE***
 
@@ -324,13 +310,16 @@ extern "C" {
         }
 \endrst */
 #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_ &);  \
     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();                                                    \

include/pybind11/embed.h

@@ -46,10 +46,13 @@
         }
  \endrst */
 #define PYBIND11_EMBEDDED_MODULE(name, variable)                                \
-    PYBIND11_DETAIL_MODULE_STATIC_DEF(name)                                     \
+    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)() {          \
-        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();                                                     \

include/pybind11/pybind11.h

@@ -868,16 +868,6 @@ class cpp_function : public function {
     }
 };
 
-class module_;
-
-PYBIND11_NAMESPACE_BEGIN(detail)
-#if PY_MAJOR_VERSION >= 3
-inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
-#else
-inline module_ create_top_level_module(const char *name, const char *doc);
-#endif
-PYBIND11_NAMESPACE_END(detail)
-
 /// Wrapper for Python extension modules
 class module_ : public object {
 public:
@@ -887,9 +877,9 @@ class module_ : public object {
     PYBIND11_DEPRECATED("Use PYBIND11_MODULE, module_::def_submodule, or module_::import instead")
     explicit module_(const char *name, const char *doc = nullptr) {
 #if PY_MAJOR_VERSION >= 3
-        *this = detail::create_top_level_module(name, doc, new PyModuleDef());
+        *this = create_extension_module(name, doc, new PyModuleDef());
 #else
-        *this = detail::create_top_level_module(name, doc);
+        *this = create_extension_module(name, doc, nullptr);
 #endif
     }
 
@@ -958,43 +948,54 @@ class module_ : public object {
 
         PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
     }
+
+#if PY_MAJOR_VERSION >= 3
+    using module_def = PyModuleDef;
+#else
+    struct module_def {};
+#endif
+
+    /** \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,
+            /* m_doc */      options::show_user_defined_docstrings() ? doc : nullptr,
+            /* m_size */     -1,
+            /* m_methods */  nullptr,
+            /* m_slots */    nullptr,
+            /* m_traverse */ nullptr,
+            /* m_clear */    nullptr,
+            /* m_free */     nullptr
+        };
+        auto m = PyModule_Create(def);
+        if (m == nullptr)
+            pybind11_fail("Internal error in module_::create_extension_module()");
+        // TODO: Should be reinterpret_steal, but Python also steals it again when returned from PyInit_...
+        return reinterpret_borrow<module_>(m);
+#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);
+        if (m == nullptr)
+            pybind11_fail("Internal error in module_::create_extension_module()");
+        return reinterpret_borrow<module_>(m);
+#endif
+    }
 };
 
 // When inside a namespace (or anywhere as long as it's not the first item on a line),
 // C++20 allows "module" to be used. This is provided for backward compatibility, and for
 // simplicity, if someone wants to use py::module for example, that is perfectly safe.
 using module = module_;
 
-PYBIND11_NAMESPACE_BEGIN(detail)
-#if PY_MAJOR_VERSION >= 3
-inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) {
-    def = new (def) PyModuleDef {  // Placement new (not an allocation).
-        /* m_base */     PyModuleDef_HEAD_INIT,
-        /* m_name */     name,
-        /* m_doc */      options::show_user_defined_docstrings() ? doc : nullptr,
-        /* m_size */     -1,
-        /* m_methods */  nullptr,
-        /* m_slots */    nullptr,
-        /* m_traverse */ nullptr,
-        /* m_clear */    nullptr,
-        /* m_free */     nullptr
-    };
-    auto m = PyModule_Create(def);
-    if (m == nullptr)
-        pybind11_fail("Internal error in detail::create_top_level_module()");
-    // TODO: Should be reinterpret_steal, but Python also steals it again when returned from PyInit_...
-    return reinterpret_borrow<module_>(m);
-}
-#else
-inline module_ create_top_level_module(const char *name, const char *doc) {
-    auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
-    if (m == nullptr)
-        pybind11_fail("Internal error in detail::create_top_level_module()");
-    return reinterpret_borrow<module_>(m);
-}
-#endif
-PYBIND11_NAMESPACE_END(detail)
-
 /// \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).

tests/test_modules.cpp

@@ -62,11 +62,8 @@ TEST_SUBMODULE(modules, m) {
         class Dupe3 { };
         class DupeException { };
 
-#if PY_MAJOR_VERSION >= 3
-        auto dm = py::detail::create_top_level_module("dummy", nullptr, new PyModuleDef);
-#else
-        auto dm = py::detail::create_top_level_module("dummy", nullptr);
-#endif
+        // 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_<Dupe1>(dm, "Dupe1");