Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate public constructors of module_ class #2552

Merged
merged 6 commits into from
Oct 9, 2020
Merged

Deprecate public constructors of module_ class #2552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cddec78
Deprecated public constructors of module
YannickJadoul Oct 5, 2020
4c41330
Turn documentation comment of module_::add_object into valid doxygen …
YannickJadoul Oct 6, 2020
7e51331
Move definition of PYBIND11_DETAIL_MODULE_STATIC_DEF and PYBIND11_DET…
YannickJadoul Oct 7, 2020
a65a38b
Move detail::create_top_level_module to module_::create_extension_mod…
YannickJadoul Oct 8, 2020
0d0b596
Throw error_already_set if module creation fails in module_::create_e…
YannickJadoul Oct 8, 2020
f0927df
Mention module_::create_extension_module in deprecation warning messa…
YannickJadoul Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/Doxyfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
23 changes: 7 additions & 16 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
44 changes: 24 additions & 20 deletions include/pybind11/embed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) { \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use PYBIND11_CATCH_INIT_EXCEPTIONS here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pour that into another PR, yes.

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)
Expand Down
77 changes: 39 additions & 38 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,32 +868,20 @@ class cpp_function : public function {
}
};


#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``
Expand Down Expand Up @@ -946,11 +934,13 @@ class module_ : public object {
*this = reinterpret_steal<module_>(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 \"" +
Expand All @@ -959,11 +949,21 @@ class module_ : public object {
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,
Expand All @@ -975,27 +975,28 @@ class module_ : public object {
/* 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<module_>(m);
}
};

// 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_;

#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).
Expand Down
3 changes: 2 additions & 1 deletion tests/test_modules.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
auto failures = py::list();

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