From 2ac5044a05dd22991b227e4755c607553b7b471d Mon Sep 17 00:00:00 2001 From: Wenzel Jakob Date: Sun, 17 Jan 2016 22:36:35 +0100 Subject: [PATCH] moved processing of cpp_function arguments out of dispatch code The cpp_function class accepts a variadic argument, which was formerly processed twice -- once at registration time, and once in the dispatch lambda function. This is not only unnecessarily slow but also leads to code bloat since it adds to the object code generated for every bound function. This change removes the second pass at dispatch time. One noteworthy change of this commit is that default arguments are now constructed (and converted to Python objects) right at declaration time. Consider the following example: py::class_("MyClass") .def("myFunction", py::arg("arg") = SomeType(123)); In this case, the change means that pybind11 must already be set up to deal with values of the type 'SomeType', or an exception will be thrown. Another change is that the "preview" of the default argument in the function signature is generated using the __repr__ special method. If it is not available in this type, the signature may not be very helpful, i.e.: | myFunction(...) | Signature : (MyClass, arg : SomeType = ) -> None One workaround (other than defining SomeType.__repr__) is to specify the human-readable preview of the default argument manually using the more cumbersome arg_t notation: py::class_("MyClass") .def("myFunction", py::arg_t("arg", SomeType(123), "SomeType(123)")); --- docs/advanced.rst | 39 ++++++ include/pybind11/cast.h | 22 ++-- include/pybind11/common.h | 14 +++ include/pybind11/pybind11.h | 238 ++++++++++++++++-------------------- 4 files changed, 175 insertions(+), 138 deletions(-) diff --git a/docs/advanced.rst b/docs/advanced.rst index 154c65f3..f8ed303c 100644 --- a/docs/advanced.rst +++ b/docs/advanced.rst @@ -826,3 +826,42 @@ When conversion fails, both directions throw the exception :class:`cast_error`. The file :file:`example/example2.cpp` contains a complete example that demonstrates passing native Python types in more detail. + +Default arguments revisited +=========================== + +The section on :ref:`default_args` previously discussed basic usage of default +arguments using pybind11. One noteworthy aspect of their implementation is that +default arguments are converted to Python objects right at declaration time. +Consider the following example: + +.. code-block:: cpp + + py::class_("MyClass") + .def("myFunction", py::arg("arg") = SomeType(123)); + +In this case, pybind11 must already be set up to deal with values of the type +``SomeType`` (via a prior instantiation of ``py::class_``), or an +exception will be thrown. + +Another aspect worth highlighting is that the "preview" of the default argument +in the function signature is generated using the object's ``__repr__`` method. +If not available, the signature may not be very helpful, e.g.: + +.. code-block:: python + + FUNCTIONS + ... + | myFunction(...) + | Signature : (MyClass, arg : SomeType = ) -> None + ... + +The first way of addressing this is by defining ``SomeType.__repr__``. +Alternatively, it is possible to specify the human-readable preview of the +default argument manually using the ``arg_t`` notation: + +.. code-block:: cpp + + py::class_("MyClass") + .def("myFunction", py::arg_t("arg", SomeType(123), "SomeType(123)")); + diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index ebdc8414..0c7efc6d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -13,6 +13,7 @@ #include "pytypes.h" #include "typeid.h" #include +#include #include NAMESPACE_BEGIN(pybind11) @@ -440,23 +441,28 @@ public: return cast(src, policy, parent, typename make_index_sequence::type()); } - static descr name(const char **keywords = nullptr, const char **values = nullptr) { - std::array names {{ + static descr name(const std::list &args = std::list()) { + std::array type_names {{ type_caster::type>::name()... }}; + auto it = args.begin(); class descr result("("); for (int i=0; iname; result += " : "; } - result += std::move(names[i]); - if (values && values[i]) { - result += " = "; - result += values[i]; + result += std::move(type_names[i]); + if (it != args.end()) { + if (it->descr) { + result += " = "; + result += it->descr; + } + ++it; } if (i+1 < size) result += ", "; + ++it; } result += ")"; return result; diff --git a/include/pybind11/common.h b/include/pybind11/common.h index bebcd072..3ed473cd 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -165,6 +165,20 @@ struct overload_hash { } }; +/// Stores information about a keyword argument +struct argument_entry { + char *name; ///< Argument name + char *descr; ///< Human-readable version of the argument value + PyObject *value; ///< Associated Python object + + argument_entry(char *name, char *descr, PyObject *value) + : name(name), descr(descr), value(value) { } + + ~argument_entry() { + free(name); free(descr); Py_XDECREF(value); + } +}; + /// Internal data struture used to track registered instances and types struct internals { std::unordered_map registered_types; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4f7d5894..34383bca 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -39,9 +39,12 @@ struct arg { /// Annotation for keyword arguments with default values template struct arg_t : public arg { - arg_t(const char *name, const T &value) : arg(name), value(value) { } + arg_t(const char *name, const T &value, const char *value_str = nullptr) + : arg(name), value(value), value_str(value_str) {} T value; + const char *value_str; }; + template arg_t arg::operator=(const T &value) { return arg_t(name, value); } /// Annotation for methods @@ -59,21 +62,36 @@ struct sibling { PyObject *value; sibling(handle value) : value(value.ptr()) { } /// Wraps an arbitrary C++ function/method/lambda function/.. into a callable Python object class cpp_function : public function { private: - /// Chained list of function entries for overloading + /// Linked list of function overloads struct function_entry { - const char *name = nullptr; - PyObject * (*impl) (function_entry *, PyObject *, PyObject *, PyObject *) = nullptr; - PyMethodDef *def = nullptr; + /// Function name and user-specified documentation string + const char *name = nullptr, *doc = nullptr; + /// List of registered keyword arguments + std::list args; + /// Pointer to lambda function which converts arguments and performs the call + PyObject * (*impl) (function_entry *, PyObject *, PyObject *) = nullptr; + /// Storage for the wrapped function pointer and captured data, if any void *data = nullptr; + /// Pointer to custom destructor for 'data' (if needed) void (*free) (void *ptr) = nullptr; - bool is_constructor = false, is_method = false; - short keywords = 0; + /// Return value policy associated with this function return_value_policy policy = return_value_policy::automatic; - std::string signature; + /// True if name == '__init__' + bool is_constructor = false; + /// Python method object + PyMethodDef *def = nullptr; + /// Pointer to class (if this is method) PyObject *class_ = nullptr; + /// Pointer to first registered function in overload chain PyObject *sibling = nullptr; - const char *doc = nullptr; + /// Pointer to next overload function_entry *next = nullptr; + + ~function_entry() { + delete def; + if (free) + free(data); + } }; function_entry *m_entry; @@ -87,88 +105,42 @@ private: template using arg_value_caster = detail::type_caster>; - template static void process_extras(const std::tuple &args, - function_entry *entry, const char **kw, const char **def) { - process_extras(args, entry, kw, def, typename detail::make_index_sequence::type()); + template static void process_extras(const std::tuple &args, function_entry *entry) { + process_extras(args, entry, typename detail::make_index_sequence::type()); } template static void process_extras(const std::tuple &args, - function_entry *entry, const char **kw, const char **def, detail::index_sequence) { - int unused[] = { 0, (process_extra(std::get(args), entry, kw, def), 0)... }; + function_entry *entry, detail::index_sequence) { + int unused[] = { 0, (process_extra(std::get(args), entry), 0)... }; (void) unused; } - template static int process_extras(const std::tuple &args, - PyObject *pyArgs, PyObject *kwargs, bool is_method) { - return process_extras(args, pyArgs, kwargs, is_method, typename detail::make_index_sequence::type()); - } - - template static int process_extras(const std::tuple &args, - PyObject *pyArgs, PyObject *kwargs, bool is_method, detail::index_sequence) { - int index = is_method ? 1 : 0, kwarg_refs = 0; - int unused[] = { 0, (process_extra(std::get(args), index, kwarg_refs, pyArgs, kwargs), 0)... }; - (void) unused; (void) index; - return kwarg_refs; - } - - static void process_extra(const char *doc, function_entry *entry, const char **, const char **) { entry->doc = doc; } - static void process_extra(const pybind11::doc &d, function_entry *entry, const char **, const char **) { entry->doc = d.value; } - static void process_extra(const pybind11::name &n, function_entry *entry, const char **, const char **) { entry->name = n.value; } - static void process_extra(const pybind11::arg &a, function_entry *entry, const char **kw, const char **) { - if (entry->is_method && entry->keywords == 0) - kw[entry->keywords++] = "self"; - kw[entry->keywords++] = a.name; + static void process_extra(const char *doc, function_entry *entry) { entry->doc = doc; } + static void process_extra(const pybind11::doc &d, function_entry *entry) { entry->doc = d.value; } + static void process_extra(const pybind11::name &n, function_entry *entry) { entry->name = n.value; } + static void process_extra(const pybind11::return_value_policy p, function_entry *entry) { entry->policy = p; } + static void process_extra(const pybind11::sibling s, function_entry *entry) { entry->sibling = s.value; } + static void process_extra(const pybind11::is_method &m, function_entry *entry) { entry->class_ = m.class_; } + static void process_extra(const pybind11::arg &a, function_entry *entry) { + if (entry->class_ && entry->args.empty()) + entry->args.emplace_back(strdup("self"), nullptr, nullptr); + entry->args.emplace_back(strdup(a.name), nullptr, nullptr); } template - static void process_extra(const pybind11::arg_t &a, function_entry *entry, const char **kw, const char **def) { - if (entry->is_method && entry->keywords == 0) - kw[entry->keywords++] = "self"; - kw[entry->keywords] = a.name; - def[entry->keywords++] = strdup(detail::to_string(a.value).c_str()); - } + static void process_extra(const pybind11::arg_t &a, function_entry *entry) { + if (entry->class_ && entry->args.empty()) + entry->args.emplace_back(strdup("self"), nullptr, nullptr); - static void process_extra(const pybind11::is_method &m, function_entry *entry, const char **, const char **) { - entry->is_method = true; - entry->class_ = m.class_; - } - static void process_extra(const pybind11::return_value_policy p, function_entry *entry, const char **, const char **) { entry->policy = p; } - static void process_extra(pybind11::sibling s, function_entry *entry, const char **, const char **) { entry->sibling = s.value; } - - template static void process_extra(T, int &, int&, PyObject *, PyObject *) { } - static void process_extra(const pybind11::arg &a, int &index, int &kwarg_refs, PyObject *args, PyObject *kwargs) { - if (kwargs) { - if (PyTuple_GET_ITEM(args, index) != nullptr) { - index++; - return; - } - PyObject *value = PyDict_GetItemString(kwargs, a.name); - if (value) { - Py_INCREF(value); - PyTuple_SetItem(args, index, value); - kwarg_refs++; - } - } - index++; - } - template - static void process_extra(const pybind11::arg_t &a, int &index, int &kwarg_refs, PyObject *args, PyObject *kwargs) { - if (PyTuple_GET_ITEM(args, index) != nullptr) { - index++; - return; - } - PyObject *value = nullptr; - if (kwargs) - value = PyDict_GetItemString(kwargs, a.name); - if (value) { - kwarg_refs++; - Py_INCREF(value); - } else { - value = detail::type_caster::type>::cast( + PyObject *obj = detail::type_caster::type>::cast( a.value, return_value_policy::automatic, nullptr); - } - PyTuple_SetItem(args, index, value); - index++; + + entry->args.emplace_back( + strdup(a.name), + strdup(a.value_str != nullptr ? a.value_str : + (const char *) ((object) handle(obj).attr("__repr__")).call().str()), + obj + ); } public: cpp_function() { } @@ -176,31 +148,22 @@ public: /// Vanilla function pointers template cpp_function(Return (*f)(Arg...), Extra&&... extra) { - struct capture { - Return (*f)(Arg...); - std::tuple extras; - }; - m_entry = new function_entry(); - m_entry->data = new capture { f, std::tuple(std::forward(extra)...) }; + m_entry->data = (void *) f; typedef arg_value_caster cast_in; typedef return_value_caster cast_out; - m_entry->impl = [](function_entry *entry, PyObject *pyArgs, PyObject *kwargs, PyObject *parent) -> PyObject * { - capture *data = (capture *) entry->data; - int kwarg_refs = process_extras(data->extras, pyArgs, kwargs, entry->is_method); + m_entry->impl = [](function_entry *entry, PyObject *pyArgs, PyObject *parent) -> PyObject * { cast_in args; - if (kwarg_refs != (kwargs ? PyDict_Size(kwargs) : 0) || !args.load(pyArgs, true)) + if (!args.load(pyArgs, true)) return (PyObject *) 1; /* Special return code: try next overload */ - return cast_out::cast(args.template call(data->f), entry->policy, parent); + return cast_out::cast(args.template call((Return (*)(Arg...)) entry->data), entry->policy, parent); }; - const int N = sizeof...(Extra) > sizeof...(Arg) ? sizeof...(Extra) : sizeof...(Arg); - std::array kw{}, def{}; - process_extras(((capture *) m_entry->data)->extras, m_entry, kw.data(), def.data()); + process_extras(std::make_tuple(std::forward(extra)...), m_entry); - detail::descr d = cast_in::name(kw.data(), def.data()); + detail::descr d = cast_in::name(m_entry->args); d += " -> "; d += std::move(cast_out::name()); @@ -238,32 +201,29 @@ private: void initialize(Func &&f, Return (*)(Arg...), Extra&&... extra) { struct capture { typename std::remove_reference::type f; - std::tuple extras; }; m_entry = new function_entry(); - m_entry->data = new capture { std::forward(f), std::tuple(std::forward(extra)...) }; + m_entry->data = new capture { std::forward(f) }; if (!std::is_trivially_destructible::value) m_entry->free = [](void *ptr) { delete (capture *) ptr; }; + else + m_entry->free = operator delete; typedef arg_value_caster cast_in; typedef return_value_caster cast_out; - m_entry->impl = [](function_entry *entry, PyObject *pyArgs, PyObject *kwargs, PyObject *parent) -> PyObject *{ - capture *data = (capture *) entry->data; - int kwarg_refs = process_extras(data->extras, pyArgs, kwargs, entry->is_method); + m_entry->impl = [](function_entry *entry, PyObject *pyArgs, PyObject *parent) -> PyObject *{ cast_in args; - if (kwarg_refs != (kwargs ? PyDict_Size(kwargs) : 0) || !args.load(pyArgs, true)) + if (!args.load(pyArgs, true)) return (PyObject *) 1; /* Special return code: try next overload */ - return cast_out::cast(args.template call(data->f), entry->policy, parent); + return cast_out::cast(args.template call(((capture *) entry->data)->f), entry->policy, parent); }; - const int N = sizeof...(Extra) > sizeof...(Arg) ? sizeof...(Extra) : sizeof...(Arg); - std::array kw{}, def{}; - process_extras(((capture *) m_entry->data)->extras, m_entry, kw.data(), def.data()); + process_extras(std::make_tuple(std::forward(extra)...), m_entry); - detail::descr d = cast_in::name(kw.data(), def.data()); + detail::descr d = cast_in::name(m_entry->args); d += " -> "; d += std::move(cast_out::name()); @@ -271,25 +231,48 @@ private: } static PyObject *dispatcher(PyObject *self, PyObject *args, PyObject *kwargs) { - function_entry *overloads = (function_entry *) PyCapsule_GetPointer(self, nullptr); - int nargs = (int) PyTuple_Size(args); - PyObject *result = nullptr; - PyObject *parent = nargs > 0 ? PyTuple_GetItem(args, 0) : nullptr; - function_entry *it = overloads; + function_entry *overloads = (function_entry *) PyCapsule_GetPointer(self, nullptr), + *it = overloads; + int nargs = (int) PyTuple_Size(args), + nkwargs = kwargs ? (int) PyDict_Size(kwargs) : 0; + PyObject *parent = nargs > 0 ? PyTuple_GetItem(args, 0) : nullptr, + *result = (PyObject *) 1; try { for (; it != nullptr; it = it->next) { PyObject *args_ = args; + int kwargs_consumed = 0; - if (it->keywords != 0 && nargs < it->keywords) { - args_ = PyTuple_New(it->keywords); - for (int i=0; iargs.size()) { + args_ = PyTuple_New(it->args.size()); + for (int i = 0; i < nargs; ++i) { PyObject *item = PyTuple_GET_ITEM(args, i); Py_INCREF(item); PyTuple_SET_ITEM(args_, i, item); } + int arg_ctr = 0; + for (auto const &it : it->args) { + int index = arg_ctr++; + if (PyTuple_GET_ITEM(args_, index)) + continue; + PyObject *value = nullptr; + if (kwargs) + value = PyDict_GetItemString(kwargs, it.name); + if (value) + kwargs_consumed++; + else if (it.value) + value = it.value; + if (value) { + Py_INCREF(value); + PyTuple_SET_ITEM(args_, index, value); + } else { + kwargs_consumed = -1; /* definite failure */ + break; + } + } } - result = it->impl(it, args_, kwargs, parent); + if (kwargs_consumed == nkwargs) + result = it->impl(it, args_, parent); if (args_ != args) { Py_DECREF(args_); @@ -318,7 +301,7 @@ private: int ctr = 0; for (function_entry *it2 = overloads; it2 != nullptr; it2 = it2->next) { msg += " "+ std::to_string(++ctr) + ". "; - msg += it2->signature; + //msg += it2->signature; XXX msg += "\n"; } PyErr_SetString(PyExc_TypeError, msg.c_str()); @@ -326,7 +309,7 @@ private: } else if (result == nullptr) { std::string msg = "Unable to convert function return value to a " "Python type! The signature was\n\t"; - msg += it->signature; + //msg += it->signature; PyErr_SetString(PyExc_TypeError, msg.c_str()); return nullptr; } else { @@ -343,18 +326,13 @@ private: static void destruct(function_entry *entry) { while (entry) { - delete entry->def; - if (entry->free) - entry->free(entry->data); - else - operator delete(entry->data); function_entry *next = entry->next; delete entry; entry = next; } } - void initialize(const detail::descr &descr, int args) { + void initialize(const detail::descr &, int args) { if (m_entry->name == nullptr) m_entry->name = ""; @@ -363,14 +341,14 @@ private: m_entry->name = "next"; #endif - if (m_entry->keywords != 0 && m_entry->keywords != args) + if (!m_entry->args.empty() && (int) m_entry->args.size() != args) throw std::runtime_error( "cpp_function(): function \"" + std::string(m_entry->name) + "\" takes " + - std::to_string(args) + " arguments, but " + std::to_string(m_entry->keywords) + + std::to_string(args) + " arguments, but " + std::to_string(m_entry->args.size()) + " pybind11::arg entries were specified!"); m_entry->is_constructor = !strcmp(m_entry->name, "__init__"); - m_entry->signature = descr.str(); + //m_entry->signature = descr.str(); // XXX #if PY_MAJOR_VERSION < 3 if (m_entry->sibling && PyMethod_Check(m_entry->sibling)) @@ -407,10 +385,10 @@ private: std::string signatures; int index = 0; function_entry *it = entry; - while (it) { /* Create pydoc it */ + while (it) { /* Create pydoc entry */ if (s_entry) signatures += std::to_string(++index) + ". "; - signatures += "Signature : " + std::string(it->signature) + "\n"; + //signatures += "Signature : " + std::string(it->signature) + "\n"; XXX if (it->doc && strlen(it->doc) > 0) signatures += "\n" + std::string(it->doc) + "\n"; if (it->next) @@ -421,7 +399,7 @@ private: if (func->m_ml->ml_doc) std::free((char *) func->m_ml->ml_doc); func->m_ml->ml_doc = strdup(signatures.c_str()); - if (entry->is_method) { + if (entry->class_) { #if PY_MAJOR_VERSION >= 3 m_ptr = PyInstanceMethod_New(m_ptr); #else @@ -895,7 +873,7 @@ NAMESPACE_END(detail) template detail::init init() { return detail::init(); }; template void implicitly_convertible() { - auto implicit_caster = [](PyObject *obj, PyTypeObject *type) -> PyObject *{ + auto implicit_caster = [](PyObject *obj, PyTypeObject *type) -> PyObject * { if (!detail::type_caster().load(obj, false)) return nullptr; tuple args(1);