Skip to content

Commit 0c5cc03

Browse files
YannickJadoulYannickJadoul
authored
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_
1 parent 71aea49 commit 0c5cc03

File tree

5 files changed

+74
-76
lines changed

5 files changed

+74
-76
lines changed

‎docs/Doxyfile

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,5 @@ QUIET = YES
1919
WARNINGS = YES
2020
WARN_IF_UNDOCUMENTED = NO
2121
PREDEFINED = DOXYGEN_SHOULD_SKIP_THIS \
22-
PY_MAJOR_VERSION=3
22+
PY_MAJOR_VERSION=3 \
23+
PYBIND11_NOINLINE

‎include/pybind11/detail/common.h

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -309,32 +309,23 @@ extern "C" {
309309
});
310310
}
311311
\endrst */
312-
#if PY_MAJOR_VERSION >= 3
313-
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
314-
static PyModuleDef PYBIND11_CONCAT(pybind11_module_def_, name);
315-
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
316-
auto m = pybind11::detail::create_top_level_module( \
317-
PYBIND11_TOSTRING(name), nullptr, \
318-
&PYBIND11_CONCAT(pybind11_module_def_, name));
319-
#else
320-
#define PYBIND11_DETAIL_MODULE_STATIC_DEF(name)
321-
#define PYBIND11_DETAIL_MODULE_CREATE(name) \
322-
auto m = pybind11::module_(PYBIND11_TOSTRING(name));
323-
#endif
324312
#define PYBIND11_MODULE(name, variable) \
325-
PYBIND11_DETAIL_MODULE_STATIC_DEF(name) \
313+
static ::pybind11::module_::module_def \
314+
PYBIND11_CONCAT(pybind11_module_def_, name); \
326315
PYBIND11_MAYBE_UNUSED \
327-
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \
316+
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
328317
PYBIND11_PLUGIN_IMPL(name) { \
329318
PYBIND11_CHECK_PYTHON_VERSION \
330319
PYBIND11_ENSURE_INTERNALS_READY \
331-
PYBIND11_DETAIL_MODULE_CREATE(name) \
320+
auto m = ::pybind11::module_::create_extension_module( \
321+
PYBIND11_TOSTRING(name), nullptr, \
322+
&PYBIND11_CONCAT(pybind11_module_def_, name)); \
332323
try { \
333324
PYBIND11_CONCAT(pybind11_init_, name)(m); \
334325
return m.ptr(); \
335326
} PYBIND11_CATCH_INIT_EXCEPTIONS \
336327
} \
337-
void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable)
328+
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable)
338329

339330

340331
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

‎include/pybind11/embed.h

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -45,26 +45,30 @@
4545
});
4646
}
4747
\endrst */
48-
#define PYBIND11_EMBEDDED_MODULE(name, variable) \
49-
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &); \
50-
static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \
51-
auto m = pybind11::module_(PYBIND11_TOSTRING(name)); \
52-
try { \
53-
PYBIND11_CONCAT(pybind11_init_, name)(m); \
54-
return m.ptr(); \
55-
} catch (pybind11::error_already_set &e) { \
56-
PyErr_SetString(PyExc_ImportError, e.what()); \
57-
return nullptr; \
58-
} catch (const std::exception &e) { \
59-
PyErr_SetString(PyExc_ImportError, e.what()); \
60-
return nullptr; \
61-
} \
62-
} \
63-
PYBIND11_EMBEDDED_MODULE_IMPL(name) \
64-
pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \
65-
(PYBIND11_TOSTRING(name), \
66-
PYBIND11_CONCAT(pybind11_init_impl_, name)); \
67-
void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module_ &variable)
48+
#define PYBIND11_EMBEDDED_MODULE(name, variable) \
49+
static ::pybind11::module_::module_def \
50+
PYBIND11_CONCAT(pybind11_module_def_, name); \
51+
static void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &); \
52+
static PyObject PYBIND11_CONCAT(*pybind11_init_wrapper_, name)() { \
53+
auto m = ::pybind11::module_::create_extension_module( \
54+
PYBIND11_TOSTRING(name), nullptr, \
55+
&PYBIND11_CONCAT(pybind11_module_def_, name)); \
56+
try { \
57+
PYBIND11_CONCAT(pybind11_init_, name)(m); \
58+
return m.ptr(); \
59+
} catch (::pybind11::error_already_set &e) { \
60+
PyErr_SetString(PyExc_ImportError, e.what()); \
61+
return nullptr; \
62+
} catch (const std::exception &e) { \
63+
PyErr_SetString(PyExc_ImportError, e.what()); \
64+
return nullptr; \
65+
} \
66+
} \
67+
PYBIND11_EMBEDDED_MODULE_IMPL(name) \
68+
::pybind11::detail::embedded_module PYBIND11_CONCAT(pybind11_module_, name) \
69+
(PYBIND11_TOSTRING(name), \
70+
PYBIND11_CONCAT(pybind11_init_impl_, name)); \
71+
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ &variable)
6872

6973

7074
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

‎include/pybind11/pybind11.h

Lines changed: 39 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -868,32 +868,20 @@ class cpp_function : public function {
868868
}
869869
};
870870

871-
872-
#if PY_MAJOR_VERSION >= 3
873-
class module_;
874-
875-
PYBIND11_NAMESPACE_BEGIN(detail)
876-
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def);
877-
PYBIND11_NAMESPACE_END(detail)
878-
#endif
879-
880871
/// Wrapper for Python extension modules
881872
class module_ : public object {
882873
public:
883874
PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check)
884875

885876
/// Create a new top-level Python module with the given name and docstring
886-
explicit module_(const char *name, const char *doc = nullptr)
877+
PYBIND11_DEPRECATED("Use PYBIND11_MODULE or module_::create_extension_module instead")
878+
explicit module_(const char *name, const char *doc = nullptr) {
887879
#if PY_MAJOR_VERSION >= 3
888-
: module_(name, doc, new PyModuleDef()) {}
880+
*this = create_extension_module(name, doc, new PyModuleDef());
889881
#else
890-
{
891-
m_ptr = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
892-
if (m_ptr == nullptr)
893-
pybind11_fail("Internal error in module_::module_()");
894-
inc_ref();
895-
}
882+
*this = create_extension_module(name, doc, nullptr);
896883
#endif
884+
}
897885

898886
/** \rst
899887
Create Python binding for a new function within the module scope. ``Func``
@@ -946,11 +934,13 @@ class module_ : public object {
946934
*this = reinterpret_steal<module_>(obj);
947935
}
948936

949-
// Adds an object to the module using the given name. Throws if an object with the given name
950-
// already exists.
951-
//
952-
// overwrite should almost always be false: attempting to overwrite objects that pybind11 has
953-
// established will, in most cases, break things.
937+
/** \rst
938+
Adds an object to the module using the given name. Throws if an object with the given name
939+
already exists.
940+
941+
``overwrite`` should almost always be false: attempting to overwrite objects that pybind11 has
942+
established will, in most cases, break things.
943+
\endrst */
954944
PYBIND11_NOINLINE void add_object(const char *name, handle obj, bool overwrite = false) {
955945
if (!overwrite && hasattr(*this, name))
956946
pybind11_fail("Error during initialization: multiple incompatible definitions with name \"" +
@@ -959,11 +949,21 @@ class module_ : public object {
959949
PyModule_AddObject(ptr(), name, obj.inc_ref().ptr() /* steals a reference */);
960950
}
961951

962-
private:
963952
#if PY_MAJOR_VERSION >= 3
964-
friend module_ detail::create_top_level_module(const char *, const char *, PyModuleDef *);
953+
using module_def = PyModuleDef;
954+
#else
955+
struct module_def {};
956+
#endif
965957

966-
explicit module_(const char *name, const char *doc, PyModuleDef *def) {
958+
/** \rst
959+
Create a new top-level module that can be used as the main module of a C extension.
960+
961+
For Python 3, ``def`` should point to a staticly allocated module_def.
962+
For Python 2, ``def`` can be a nullptr and is completely ignored.
963+
\endrst */
964+
static module_ create_extension_module(const char *name, const char *doc, module_def *def) {
965+
#if PY_MAJOR_VERSION >= 3
966+
// module_def is PyModuleDef
967967
def = new (def) PyModuleDef { // Placement new (not an allocation).
968968
/* m_base */ PyModuleDef_HEAD_INIT,
969969
/* m_name */ name,
@@ -975,27 +975,28 @@ class module_ : public object {
975975
/* m_clear */ nullptr,
976976
/* m_free */ nullptr
977977
};
978-
m_ptr = PyModule_Create(def);
979-
if (m_ptr == nullptr)
980-
pybind11_fail("Internal error in module_::module_()");
981-
inc_ref();
982-
}
978+
auto m = PyModule_Create(def);
979+
#else
980+
// Ignore module_def *def; only necessary for Python 3
981+
(void) def;
982+
auto m = Py_InitModule3(name, nullptr, options::show_user_defined_docstrings() ? doc : nullptr);
983983
#endif
984+
if (m == nullptr) {
985+
if (PyErr_Occurred())
986+
throw error_already_set();
987+
pybind11_fail("Internal error in module_::create_extension_module()");
988+
}
989+
// TODO: Sould be reinterpret_steal for Python 3, but Python also steals it again when returned from PyInit_...
990+
// For Python 2, reinterpret_borrow is correct.
991+
return reinterpret_borrow<module_>(m);
992+
}
984993
};
985994

986995
// When inside a namespace (or anywhere as long as it's not the first item on a line),
987996
// C++20 allows "module" to be used. This is provided for backward compatibility, and for
988997
// simplicity, if someone wants to use py::module for example, that is perfectly safe.
989998
using module = module_;
990999

991-
#if PY_MAJOR_VERSION >= 3
992-
PYBIND11_NAMESPACE_BEGIN(detail)
993-
inline module_ create_top_level_module(const char *name, const char *doc, PyModuleDef *def) {
994-
return module_(name, doc, def);
995-
}
996-
PYBIND11_NAMESPACE_END(detail)
997-
#endif
998-
9991000
/// \ingroup python_builtins
10001001
/// Return a dictionary representing the global variables in the current execution frame,
10011002
/// or ``__main__.__dict__`` if there is no frame (usually when the interpreter is embedded).

‎tests/test_modules.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,8 @@ TEST_SUBMODULE(modules, m) {
6262
class Dupe3 { };
6363
class DupeException { };
6464

65-
auto dm = py::module_("dummy");
65+
// Go ahead and leak, until we have a non-leaking py::module_ constructor
66+
auto dm = py::module_::create_extension_module("dummy", nullptr, new py::module_::module_def);
6667
auto failures = py::list();
6768

6869
py::class_<Dupe1>(dm, "Dupe1");

0 commit comments

Comments
 (0)