Pack all function call data into a single struct

jagerman · jagerman · commit 37cf37b299ff · 2017-01-30T13:34:38.000-05:00
This cleans up the previous commit slightly by further reducing the function call arguments to a single struct (containing the function_record, arguments vector, and parent). Although this doesn't currently change anything, it does allow for future functionality to have a place for precalls to store temporary objects that need to be destroyed after a function call (whether or not the call succeeds). As a concrete example, with this change pybind#625 could be easily implemented (I think) by adding a std::unique_ptr<gil_scoped_release> member to the `function_call` struct with a precall that actually constructs it. Without this, the precall can't do that: the postcall won't be invoked if the call throws an exception. This doesn't seems to affect the .so size noticeably (either way).
diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
@@ -64,7 +64,8 @@ struct undefined_t;
 template <op_id id, op_type ot, typename L = undefined_t, typename R = undefined_t> struct op_;
 template <typename... Args> struct init;
 template <typename... Args> struct init_alias;
-inline void keep_alive_impl(size_t Nurse, size_t Patient, function_arguments args, handle ret);
+struct function_call;
+inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret);
 
 /// Internal data structure which holds metadata about a keyword argument
 struct argument_record {
@@ -95,7 +96,7 @@ struct function_record {
     std::vector<argument_record> args;
 
     /// Pointer to lambda function which converts arguments and performs the actual call
-    handle (*impl) (function_record *, function_arguments, handle) = nullptr;
+    handle (*impl) (function_call &) = nullptr;
 
     /// Storage for the wrapped function pointer and captured data, if any
     void *data[3] = { };
@@ -204,6 +205,22 @@ struct type_record {
     }
 };
 
+/// Internal data associated with a single function call
+struct function_call {
+    function_call(const function_record &f, handle p) : func{f}, parent{p} {
+        args.reserve(f.nargs);
+    }
+
+    /// The function data:
+    const function_record &func;
+
+    /// Arguments passed to the function:
+    std::vector<handle> args;
+
+    /// The parent, if any
+    handle parent;
+};
+
 /**
  * Partial template specializations to process custom attributes provided to
  * cpp_function_ and class_. These are either used to initialize the respective
@@ -216,8 +233,8 @@ template <typename T> struct process_attribute_default {
     /// Default implementation: do nothing
     static void init(const T &, function_record *) { }
     static void init(const T &, type_record *) { }
-    static void precall(function_arguments) { }
-    static void postcall(function_arguments, handle) { }
+    static void precall(function_call &) { }
+    static void postcall(function_call &, handle) { }
 };
 
 /// Process an attribute specifying the function's name
@@ -345,13 +362,13 @@ struct process_attribute<arithmetic> : process_attribute_default<arithmetic> {};
  */
 template <size_t Nurse, size_t Patient> struct process_attribute<keep_alive<Nurse, Patient>> : public process_attribute_default<keep_alive<Nurse, Patient>> {
     template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
-    static void precall(function_arguments args) { keep_alive_impl(Nurse, Patient, args, handle()); }
+    static void precall(function_call &call) { keep_alive_impl(Nurse, Patient, call, handle()); }
     template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
-    static void postcall(function_arguments, handle) { }
+    static void postcall(function_call &, handle) { }
     template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
-    static void precall(function_arguments) { }
+    static void precall(function_call &) { }
     template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
-    static void postcall(function_arguments args, handle ret) { keep_alive_impl(Nurse, Patient, args, ret); }
+    static void postcall(function_call &call, handle ret) { keep_alive_impl(Nurse, Patient, call, ret); }
 };
 
 /// Recursively iterate over variadic template arguments
@@ -364,12 +381,12 @@ template <typename... Args> struct process_attributes {
         int unused[] = { 0, (process_attribute<typename std::decay<Args>::type>::init(args, r), 0) ... };
         ignore_unused(unused);
     }
-    static void precall(function_arguments fn_args) {
-        int unused[] = { 0, (process_attribute<typename std::decay<Args>::type>::precall(fn_args), 0) ... };
+    static void precall(function_call &call) {
+        int unused[] = { 0, (process_attribute<typename std::decay<Args>::type>::precall(call), 0) ... };
         ignore_unused(unused);
     }
-    static void postcall(function_arguments fn_args, handle fn_ret) {
-        int unused[] = { 0, (process_attribute<typename std::decay<Args>::type>::postcall(fn_args, fn_ret), 0) ... };
+    static void postcall(function_call &call, handle fn_ret) {
+        int unused[] = { 0, (process_attribute<typename std::decay<Args>::type>::postcall(call, fn_ret), 0) ... };
         ignore_unused(unused);
     }
 };
diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
@@ -1220,12 +1220,11 @@ NAMESPACE_BEGIN(detail)
 // forward declaration
 struct function_record;
 
-using function_arguments = const std::vector<handle> &;
-
 /// Helper class which loads arguments for C++ functions called from Python
 template <typename... Args>
 class argument_loader {
     using indices = make_index_sequence<sizeof...(Args)>;
+    using function_arguments = const std::vector<handle> &;
 
     template <typename Arg> using argument_is_args   = std::is_same<intrinsic_t<Arg>, args>;
     template <typename Arg> using argument_is_kwargs = std::is_same<intrinsic_t<Arg>, kwargs>;
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -118,31 +118,31 @@ class cpp_function : public function {
                       "The number of named arguments does not match the function signature");
 
         /* Dispatch code which converts function arguments and performs the actual function call */
-        rec->impl = [](detail::function_record *rec, detail::function_arguments args, handle parent) -> handle {
+        rec->impl = [](detail::function_call &call) -> handle {
             cast_in args_converter;
 
             /* Try to cast the function arguments into the C++ domain */
-            if (!args_converter.load_args(args))
+            if (!args_converter.load_args(call.args))
                 return PYBIND11_TRY_NEXT_OVERLOAD;
 
             /* Invoke call policy pre-call hook */
-            detail::process_attributes<Extra...>::precall(args);
+            detail::process_attributes<Extra...>::precall(call);
 
             /* Get a pointer to the capture object */
-            capture *cap = (capture *) (sizeof(capture) <= sizeof(rec->data)
-                                        ? &rec->data : rec->data[0]);
+            capture *cap = (capture *) (sizeof(capture) <= sizeof(call.func.data)
+                                        ? &call.func.data : call.func.data[0]);
 
             /* Override policy for rvalues -- always move */
             constexpr auto is_rvalue = !std::is_pointer<Return>::value
                                        && !std::is_lvalue_reference<Return>::value;
-            const auto policy = is_rvalue ? return_value_policy::move : rec->policy;
+            const auto policy = is_rvalue ? return_value_policy::move : call.func.policy;
 
             /* Perform the function call */
             handle result = cast_out::cast(args_converter.template call<Return>(cap->f),
-                                           policy, parent);
+                                           policy, call.parent);
 
             /* Invoke call policy post-call hook */
-            detail::process_attributes<Extra...>::postcall(args, result);
+            detail::process_attributes<Extra...>::postcall(call, result);
 
             return result;
         };
@@ -381,9 +381,11 @@ class cpp_function : public function {
 
     /// Main dispatch logic for calls to functions bound using pybind11
     static PyObject *dispatcher(PyObject *self, PyObject *args_in, PyObject *kwargs_in) {
+        using namespace detail;
+
         /* Iterator over the list of potentially admissible overloads */
-        detail::function_record *overloads = (detail::function_record *) PyCapsule_GetPointer(self, nullptr),
-                                *it = overloads;
+        function_record *overloads = (function_record *) PyCapsule_GetPointer(self, nullptr),
+                        *it = overloads;
 
         /* Need to know how many arguments + keyword arguments there are to pick the right overload */
         const size_t n_args_in = (size_t) PyTuple_GET_SIZE(args_in);
@@ -411,18 +413,18 @@ class cpp_function : public function {
                    result other than PYBIND11_TRY_NEXT_OVERLOAD.
                  */
 
-                size_t pos_args = it->nargs;    // Number of positional arguments that we need
-                if (it->has_args) --pos_args;   // (but don't count py::args
-                if (it->has_kwargs) --pos_args; //  or py::kwargs)
+                function_record &func = *it;
+                size_t pos_args = func.nargs;    // Number of positional arguments that we need
+                if (func.has_args) --pos_args;   // (but don't count py::args
+                if (func.has_kwargs) --pos_args; //  or py::kwargs)
 
-                if (!it->has_args && n_args_in > pos_args)
+                if (!func.has_args && n_args_in > pos_args)
                     continue; // Too many arguments for this overload
 
-                if (n_args_in < pos_args && it->args.size() < pos_args)
+                if (n_args_in < pos_args && func.args.size() < pos_args)
                     continue; // Not enough arguments given, and not enough defaults to fill in the blanks
 
-                std::vector<handle> pass_args;
-                pass_args.reserve(it->nargs);
+                function_call call(func, parent);
 
                 size_t args_to_copy = std::min(pos_args, n_args_in);
                 size_t args_copied = 0;
@@ -440,7 +442,7 @@ class cpp_function : public function {
                                     std::string(it->args[args_copied].name) + "'");
                     }
 
-                    pass_args.push_back(PyTuple_GET_ITEM(args_in, args_copied));
+                    call.args.push_back(PyTuple_GET_ITEM(args_in, args_copied));
                 }
 
                 // We'll need to copy this if we steal some kwargs for defaults
@@ -470,7 +472,7 @@ class cpp_function : public function {
                         }
 
                         if (value)
-                            pass_args.push_back(value);
+                            call.args.push_back(value);
                         else
                             break;
                     }
@@ -502,22 +504,26 @@ class cpp_function : public function {
                             extra_args[i] = item.inc_ref().ptr();
                         }
                     }
-                    pass_args.push_back(extra_args);
+                    call.args.push_back(extra_args);
                 }
 
                 // 4b. If we have a py::kwargs, pass on any remaining kwargs
                 if (it->has_kwargs) {
                     if (!kwargs.ptr())
                         kwargs = dict(); // If we didn't get one, send an empty one
-                    pass_args.push_back(kwargs);
+                    call.args.push_back(kwargs);
                 }
 
-                // 5. Put everything in a big tuple.  Not technically step 5, we've been building it
-                // in `pass_args` all along.
+                // 5. Put everything in a vector.  Not technically step 5, we've been building it
+                // in `call.args` all along.
+                #if !defined(NDEBUG)
+                if (call.args.size() != call.func.nargs)
+                    pybind11_fail("Internal error: function call dispatcher inserted wrong number of arguments!");
+                #endif
 
                 // 6. Call the function.
                 try {
-                    result = it->impl(it, pass_args, parent);
+                    result = it->impl(call);
                 } catch (reference_cast_error &) {
                     result = PYBIND11_TRY_NEXT_OVERLOAD;
                 }
@@ -541,7 +547,7 @@ class cpp_function : public function {
                 - delegate translation to the next translator by throwing a new type of exception. */
 
             auto last_exception = std::current_exception();
-            auto &registered_exception_translators = pybind11::detail::get_internals().registered_exception_translators;
+            auto &registered_exception_translators = get_internals().registered_exception_translators;
             for (auto& translator : registered_exception_translators) {
                 try {
                     translator(last_exception);
@@ -564,7 +570,7 @@ class cpp_function : public function {
                 " arguments. The following argument types are supported:\n";
 
             int ctr = 0;
-            for (detail::function_record *it2 = overloads; it2 != nullptr; it2 = it2->next) {
+            for (function_record *it2 = overloads; it2 != nullptr; it2 = it2->next) {
                 msg += "    "+ std::to_string(++ctr) + ". ";
 
                 bool wrote_sig = false;
@@ -609,7 +615,7 @@ class cpp_function : public function {
             if (overloads->is_constructor) {
                 /* When a constructor ran successfully, the corresponding
                    holder type (e.g. std::unique_ptr) must still be initialized. */
-                auto tinfo = detail::get_type_info(Py_TYPE(parent.ptr()));
+                auto tinfo = get_type_info(Py_TYPE(parent.ptr()));
                 tinfo->init_holder(parent.ptr(), nullptr);
             }
             return result.ptr();
@@ -1477,10 +1483,10 @@ inline void keep_alive_impl(handle nurse, handle patient) {
     (void) wr.release();
 }
 
-PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_arguments args, handle ret) {
+PYBIND11_NOINLINE inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
     keep_alive_impl(
-        Nurse   == 0 ? ret : Nurse   <= args.size() ? args[Nurse   - 1] : handle(),
-        Patient == 0 ? ret : Patient <= args.size() ? args[Patient - 1] : handle()
+        Nurse   == 0 ? ret : Nurse   <= call.args.size() ? call.args[Nurse   - 1] : handle(),
+        Patient == 0 ? ret : Patient <= call.args.size() ? call.args[Patient - 1] : handle()
     );
 }
 
