interrogate: refactor default argument handling

dtool/src/cppparser/cppExpression.cxx

@@ -25,6 +25,7 @@
 #include "cppFunctionGroup.h"
 #include "cppFunctionType.h"
 #include "cppClosureType.h"
+#include "cppReferenceType.h"
 #include "cppStructType.h"
 #include "cppBison.h"
 #include "pdtoa.h"
@@ -260,12 +261,12 @@ CPPExpression(CPPIdentifier *ident, CPPScope *current_scope,
       _u._variable = inst;
       return;
     }
-    CPPFunctionGroup *fgroup = decl->as_function_group();
+    /*CPPFunctionGroup *fgroup = decl->as_function_group();
     if (fgroup != nullptr) {
       _type = T_function;
       _u._fgroup = fgroup;
       return;
-    }
+    }*/
   }
 
   _type = T_unknown_ident;
@@ -1232,6 +1233,61 @@ determine_type() const {
   return nullptr;  // Compiler kludge; can't get here.
 }
 
+/**
+ * Returns true if this is an lvalue expression.
+ */
+bool CPPExpression::
+is_lvalue() const {
+  switch (_type) {
+  case T_variable:
+  case T_function:
+  case T_unknown_ident:
+    return true;
+
+  case T_typecast:
+  case T_static_cast:
+  case T_dynamic_cast:
+  case T_const_cast:
+  case T_reinterpret_cast:
+    {
+      CPPReferenceType *ref_type = _u._typecast._to->as_reference_type();
+      return ref_type != nullptr && ref_type->_value_category == CPPReferenceType::VC_lvalue;
+    }
+
+  case T_unary_operation:
+    if (_u._op._operator == 'f') {
+      // A function returning an lvalue reference.
+      CPPType *return_type = determine_type();
+      if (return_type != nullptr) {
+        CPPReferenceType *ref_type = return_type->as_reference_type();
+        return ref_type != nullptr && ref_type->_value_category == CPPReferenceType::VC_lvalue;
+      }
+    }
+    return _u._op._operator == PLUSPLUS
+        || _u._op._operator == MINUSMINUS
+        || _u._op._operator == '*';
+
+  case T_binary_operation:
+    if (_u._op._operator == ',') {
+      CPPReferenceType *ref_type = _u._op._op2->as_reference_type();
+      return ref_type != nullptr && ref_type->_value_category == CPPReferenceType::VC_lvalue;
+    }
+    return (_u._op._operator == POINTSAT || _u._op._operator == ',');
+
+  case T_trinary_operation:
+    return _u._op._op2->is_lvalue() && _u._op._op3->is_lvalue();
+
+  case T_literal:
+  case T_raw_literal:
+    return true;
+
+  default:
+    break;
+  }
+
+  return false;
+}
+
 /**
  * Returns true if this declaration is an actual, factual declaration, or
  * false if some part of the declaration depends on a template parameter which

dtool/src/cppparser/cppExpression.h

@@ -133,6 +133,7 @@ public:
 
   Result evaluate() const;
   CPPType *determine_type() const;
+  bool is_lvalue() const;
   bool is_tbd() const;
 
   virtual bool is_fully_specified() const;

dtool/src/interrogate/interfaceMakerPythonNative.cxx

@@ -4078,13 +4078,9 @@ write_coerce_constructor(ostream &out, Object *obj, bool is_const) {
 
 /**
  * Special case optimization: if the last map is a subset of the map before
- * it, and the last parameter is only a simple parameter type (that we have
- * special default argument handling for), we can merge the cases.  When this
- * happens, we can make use of a special feature of PyArg_ParseTuple for
- * handling of these last few default arguments.  This doesn't work well for
- * all types of default expressions, though, hence the need for this elaborate
- * checking mechanism down here, which goes in parallel with the actual
- * optional arg handling logic in write_function_instance.
+ * it, we can merge the cases.  When this happens, we can make use of a
+ * special feature of PyArg_ParseTuple for handling of these last few default
+ * arguments.
  *
  * This isn't just to help reduce the amount of generated code; it also
  * enables arbitrary selection of keyword arguments for many functions, ie.
@@ -4095,14 +4091,6 @@ write_coerce_constructor(ostream &out, Object *obj, bool is_const) {
  * Thanks to this mechanism, we can call it like so:
  *
  * func(c=True, d=".")
- *
- * The return value is the minimum of the number of maximum arguments.
- *
- * Sorry, let me try that again: it returns the largest number of arguments
- * for which the overloads will be separated out rather than handled via the
- * special default handling mechanism.  Or something.
- *
- * Please don't hate me.
  */
 int InterfaceMakerPythonNative::
 collapse_default_remaps(std::map<int, std::set<FunctionRemap *> > &map_sets,
@@ -4118,70 +4106,8 @@ collapse_default_remaps(std::map<int, std::set<FunctionRemap *> > &map_sets,
     if (std::includes(rmi_next->second.begin(), rmi_next->second.end(),
                       rmi->second.begin(), rmi->second.end())) {
 
-      // Check if the nth argument is something we can easily create a default
-      // for.
-      std::set<FunctionRemap *>::iterator sii;
-      for (sii = rmi->second.begin(); sii != rmi->second.end(); ++sii) {
-        FunctionRemap *remap = (*sii);
-        size_t pn = (size_t)rmi->first;
-        if (!remap->_has_this || remap->_type == FunctionRemap::T_constructor) {
-          --pn;
-        }
-        nassertd(pn < remap->_parameters.size()) goto abort_iteration;
-
-        ParameterRemap *param = remap->_parameters[pn]._remap;
-        CPPType *type = param->get_new_type();
-
-        if (param->new_type_is_atomic_string()) {
-          CPPType *orig_type = param->get_orig_type();
-          if (TypeManager::is_char_pointer(orig_type)) {
-          } else if (TypeManager::is_wchar_pointer(orig_type)) {
-            goto abort_iteration;
-          } else if (TypeManager::is_wstring(orig_type)) {
-            goto abort_iteration;
-          } else if (TypeManager::is_const_ptr_to_basic_string_wchar(orig_type)) {
-            goto abort_iteration;
-          } else {
-            // Regular strings are OK if the default argument is a string
-            // literal or the default string constructor, since those are
-            // trivial to handle.  This actually covers almost all of the
-            // cases of default string args.
-            CPPExpression::Type expr_type = param->get_default_value()->_type;
-            if (expr_type != CPPExpression::T_default_construct &&
-                expr_type != CPPExpression::T_string) {
-              goto abort_iteration;
-            }
-          }
-        } else if (TypeManager::is_integer(type)) {
-        } else if (TypeManager::is_float(type)) {
-        } else if (TypeManager::is_const_char_pointer(type)) {
-        } else if (TypeManager::is_pointer_to_PyTypeObject(type)) {
-        } else if (TypeManager::is_pointer_to_PyStringObject(type)) {
-        } else if (TypeManager::is_pointer_to_PyUnicodeObject(type)) {
-        } else if (TypeManager::is_pointer_to_PyObject(type)) {
-        } else if (TypeManager::is_pointer_to_Py_buffer(type)) {
-          goto abort_iteration;
-        } else if (TypeManager::is_pointer_to_simple(type)) {
-          goto abort_iteration;
-        } else if (TypeManager::is_pointer(type)) {
-          // I'm allowing other pointer types, but only if the expression
-          // happens to evaluate to a numeric constant (which will likely only
-          // be NULL). There are too many issues to resolve right now with
-          // allowing more complex default expressions, including issues in
-          // the C++ parser (but the reader is welcome to give it a try!)
-          CPPExpression::Result res = param->get_default_value()->evaluate();
-          if (res._type != CPPExpression::RT_integer &&
-              res._type != CPPExpression::RT_pointer) {
-            goto abort_iteration;
-          }
-        } else {
-          goto abort_iteration;
-        }
-      }
-
       // rmi_next has a superset of the remaps in rmi, and we are going to
-      // erase rmi_next, so put all the remaps in rmi.  rmi->second =
-      // rmi_next->second;
+      // erase rmi_next, so put all the remaps in rmi.
 
       max_required_args = rmi_next->first;
       rmi = rmi_next;
@@ -4191,7 +4117,6 @@ collapse_default_remaps(std::map<int, std::set<FunctionRemap *> > &map_sets,
     }
   }
 
-abort_iteration:
   // Now erase the other remap sets.  Reverse iterators are weird, we first
   // need to get forward iterators and decrement them by one.
   std::map<int, std::set<FunctionRemap *> >::iterator erase_begin, erase_end;
@@ -4738,6 +4663,7 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       "(" + orig_type->get_local_name(&parser) + ")" + param_name;
 
     string default_expr;
+    const char *null_assign = "";
 
     if (is_optional) {
       // If this is an optional argument, PyArg_ParseTuple will leave the
@@ -4747,6 +4673,7 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       default_expr_str << " = ";
       default_value->output(default_expr_str, 0, &parser, false);
       default_expr = default_expr_str.str();
+      null_assign = " = nullptr";
 
       // We should only ever have to consider optional arguments for functions
       // taking a variable number of arguments.
@@ -4776,14 +4703,30 @@ write_function_instance(ostream &out, FunctionRemap *remap,
         expected_params += "str";
 
       } else if (TypeManager::is_wchar_pointer(orig_type)) {
-        indent(out, indent_level) << "#if PY_VERSION_HEX >= 0x03020000\n";
-        indent(out, indent_level) << "PyObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#else\n";
-        indent(out, indent_level) << "PyUnicodeObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#endif\n";
+        out << "#if PY_VERSION_HEX >= 0x03020000\n";
+        indent(out, indent_level) << "PyObject *" << param_name << null_assign << ";\n";
+        out << "#else\n";
+        indent(out, indent_level) << "PyUnicodeObject *" << param_name << null_assign << ";\n";
+        out << "#endif\n";
         format_specifiers += "U";
         parameter_list += ", &" + param_name;
 
+        if (is_optional) {
+          extra_convert
+            << "wchar_t *" << param_name << "_str;\n"
+            << "if (" << param_name << " != nullptr) {\n"
+            << "#if PY_VERSION_HEX >= 0x03030000\n"
+            << "  " << param_name << "_str = PyUnicode_AsWideCharString(" << param_name << ", nullptr);\n"
+            << "#else"
+            << "Py_ssize_t " << param_name << "_len = PyUnicode_GET_SIZE(" << param_name << ");\n"
+            << "  " << param_name << "_str = (wchar_t *)alloca(sizeof(wchar_t) * (" + param_name + "_len + 1));\n"
+            << "PyUnicode_AsWideChar(" << param_name << ", " << param_name << "_str, " << param_name << "_len);\n"
+            << param_name << "_str[" << param_name << "_len] = 0;\n"
+            << "#endif\n"
+            << "} else {\n"
+            << "  " << param_name << "_str" << default_expr << ";\n"
+            << "}\n";
+        } else {
           extra_convert
             << "#if PY_VERSION_HEX >= 0x03030000\n"
             << "wchar_t *" << param_name << "_str = PyUnicode_AsWideCharString(" << param_name << ", nullptr);\n"
@@ -4793,6 +4736,7 @@ write_function_instance(ostream &out, FunctionRemap *remap,
             << "PyUnicode_AsWideChar(" << param_name << ", " << param_name << "_str, " << param_name << "_len);\n"
             << param_name << "_str[" << param_name << "_len] = 0;\n"
             << "#endif\n";
+        }
 
         pexpr_string = param_name + "_str";
 
@@ -4803,15 +4747,36 @@ write_function_instance(ostream &out, FunctionRemap *remap,
 
         expected_params += "unicode";
 
-      } else if (TypeManager::is_wstring(orig_type)) {
-        indent(out, indent_level) << "#if PY_VERSION_HEX >= 0x03020000\n";
-        indent(out, indent_level) << "PyObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#else\n";
-        indent(out, indent_level) << "PyUnicodeObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#endif\n";
+      } else if (TypeManager::is_wstring(orig_type) ||
+                 TypeManager::is_const_ptr_to_basic_string_wchar(orig_type)) {
+        out << "#if PY_VERSION_HEX >= 0x03020000\n";
+        indent(out, indent_level) << "PyObject *" << param_name << null_assign << ";\n";
+        out << "#else\n";
+        indent(out, indent_level) << "PyUnicodeObject *" << param_name << null_assign << ";\n";
+        out << "#endif\n";
         format_specifiers += "U";
         parameter_list += ", &" + param_name;
 
+        if (is_optional) {
+          extra_convert
+            << "Py_ssize_t " << param_name << "_len;\n"
+            << "wchar_t *" << param_name << "_str;\n"
+            << "std::wstring " << param_name << "_wstr;\n"
+            << "if (" << param_name << " != nullptr) {\n"
+            << "#if PY_VERSION_HEX >= 0x03030000\n"
+            << "  " << param_name << "_str = PyUnicode_AsWideCharString("
+            << param_name << ", &" << param_name << "_len);\n"
+            << "#else\n"
+            << "  " << param_name << "_len = PyUnicode_GET_SIZE(" << param_name << ");\n"
+            << "  " << param_name << "_str = (wchar_t *)alloca(sizeof(wchar_t) * (" + param_name + "_len + 1));\n"
+            << "PyUnicode_AsWideChar(" << param_name << ", " << param_name << "_str, " << param_name << "_len);\n"
+            << "#endif\n"
+            << "  " << param_name << "_wstr.assign(" << param_name << "_str, " << param_name << "_len);\n"
+            << "} else {\n"
+            << "  " << param_name << "_wstr" << default_expr << ";\n"
+            << "}\n";
+          pexpr_string = "std::move(" + param_name + "_wstr)";
+        } else {
           extra_convert
             << "#if PY_VERSION_HEX >= 0x03030000\n"
             << "Py_ssize_t " << param_name << "_len;\n"
@@ -4822,37 +4787,8 @@ write_function_instance(ostream &out, FunctionRemap *remap,
             << "wchar_t *" << param_name << "_str = (wchar_t *)alloca(sizeof(wchar_t) * (" + param_name + "_len + 1));\n"
             << "PyUnicode_AsWideChar(" << param_name << ", " << param_name << "_str, " << param_name << "_len);\n"
             << "#endif\n";
-
-        pexpr_string = param_name + "_str, " + param_name + "_len";
-
-        extra_cleanup
-          << "#if PY_VERSION_HEX >= 0x03030000\n"
-          << "PyMem_Free(" << param_name << "_str);\n"
-          << "#endif\n";
-
-        expected_params += "unicode";
-
-      } else if (TypeManager::is_const_ptr_to_basic_string_wchar(orig_type)) {
-        indent(out, indent_level) << "#if PY_VERSION_HEX >= 0x03020000\n";
-        indent(out, indent_level) << "PyObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#else\n";
-        indent(out, indent_level) << "PyUnicodeObject *" << param_name << ";\n";
-        indent(out, indent_level) << "#endif\n";
-        format_specifiers += "U";
-        parameter_list += ", &" + param_name;
-
-        extra_convert
-          << "#if PY_VERSION_HEX >= 0x03030000\n"
-          << "Py_ssize_t " << param_name << "_len;\n"
-          << "wchar_t *" << param_name << "_str = PyUnicode_AsWideCharString("
-          << param_name << ", &" << param_name << "_len);\n"
-          << "#else\n"
-          << "Py_ssize_t " << param_name << "_len = PyUnicode_GET_SIZE(" << param_name << ");\n"
-          << "wchar_t *" << param_name << "_str = (wchar_t *)alloca(sizeof(wchar_t) * (" + param_name + "_len + 1));\n"
-          << "PyUnicode_AsWideChar(" << param_name << ", " << param_name << "_str, " << param_name << "_len);\n"
-          << "#endif\n";
-
           pexpr_string = param_name + "_str, " + param_name + "_len";
+        }
 
         extra_cleanup
           << "#if PY_VERSION_HEX >= 0x03030000\n"
@@ -5019,11 +4955,11 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       only_pyobjects = false;
 
     } else if (TypeManager::is_wchar(type)) {
-      indent(out, indent_level) << "#if PY_VERSION_HEX >= 0x03020000\n";
+      out << "#if PY_VERSION_HEX >= 0x03020000\n";
       indent(out, indent_level) << "PyObject *" << param_name << ";\n";
-      indent(out, indent_level) << "#else\n";
+      out << "#else\n";
       indent(out, indent_level) << "PyUnicodeObject *" << param_name << ";\n";
-      indent(out, indent_level) << "#endif\n";
+      out << "#endif\n";
       format_specifiers += "U";
       parameter_list += ", &" + param_name;
 
@@ -5348,17 +5284,36 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       if (args_type == AT_single_arg) {
         param_name = "arg";
       } else {
-        indent(out, indent_level) << "PyObject *" << param_name << ";\n";
+        indent(out, indent_level) << "PyObject *" << param_name << null_assign << ";\n";
         format_specifiers += "O";
         parameter_list += ", &" + param_name;
       }
       indent(out, indent_level) << "Py_buffer " << param_name << "_view;\n";
 
+      if (is_optional) {
+        indent(out, indent_level) << "Py_buffer *" << param_name << "_viewp;\n";
+
+        extra_convert
+          << "bool " << param_name << "_success;\n"
+          << "if (" << param_name << " != nullptr) {\n"
+          << "  " << param_name << "_success = (PyObject_GetBuffer("
+          << param_name << ", &" << param_name << "_view, PyBUF_FULL) == 0);\n"
+          << "  " << param_name << "_viewp = &" << param_name << "_view;\n"
+          << "} else {\n"
+          << "  " << param_name << "_viewp" << default_expr << ";\n"
+          << "  " << param_name << "_success = true;\n"
+          << "}\n";
+
+        extra_param_check << " && " << param_name << "_success";
+        pexpr_string = param_name + "_viewp";
+        extra_cleanup << "if (" << param_name << " != nullptr) PyBuffer_Release(&" << param_name << "_view);\n";
+      } else {
         extra_param_check << " && PyObject_GetBuffer("
                           << param_name << ", &"
                           << param_name << "_view, PyBUF_FULL) == 0";
         pexpr_string = "&" + param_name + "_view";
         extra_cleanup << "PyBuffer_Release(&" << param_name << "_view);\n";
+      }
       expected_params += "buffer";
       may_raise_typeerror = true;
       clear_error = true;
@@ -5367,7 +5322,7 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       if (args_type == AT_single_arg) {
         param_name = "arg";
       } else {
-        indent(out, indent_level) << "PyObject *" << param_name << ";\n";
+        indent(out, indent_level) << "PyObject *" << param_name << null_assign << ";\n";
         format_specifiers += "O";
         parameter_list += ", &" + param_name;
       }
@@ -5496,18 +5451,15 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       if (args_type == AT_single_arg) {
         param_name = "arg";
       } else {
-        indent(out, indent_level) << "PyObject *" << param_name;
-        if (is_optional) {
-          out << " = nullptr";
-        }
-        out << ";\n";
+        indent(out, indent_level) << "PyObject *" << param_name << null_assign << ";\n";
         format_specifiers += "O";
         parameter_list += ", &" + param_name;
       }
 
       // If the default value is NULL, we also accept a None value.
       bool maybe_none = false;
-      if (default_value != nullptr && (return_flags & RF_coerced) == 0) {
+      if (default_value != nullptr && (return_flags & RF_coerced) == 0 &&
+          TypeManager::is_pointer(orig_type)) {
         CPPExpression::Result res = param->get_default_value()->evaluate();
         if (res._type == CPPExpression::RT_integer ||
             res._type == CPPExpression::RT_pointer) {
@@ -5576,8 +5528,11 @@ write_function_instance(ostream &out, FunctionRemap *remap,
               << "if (" << param_name << " != nullptr && " << param_name << " != Py_None) {\n"
               << "  " << param_name << "_this";
           } else if (is_optional) {
+            if (TypeManager::is_pointer(orig_type)) {
+              extra_convert << default_expr;
+            }
             extra_convert
-              << default_expr << ";\n"
+              << ";\n"
               << "if (" << param_name << " != nullptr) {\n"
               << "  " << param_name << "_this";
           } else if (maybe_none) {
@@ -5590,7 +5545,13 @@ write_function_instance(ostream &out, FunctionRemap *remap,
           extra_convert << " = Dtool_Coerce_" + make_safe_name(class_name) +
             "(" + param_name + ", " + param_name + "_local);\n";
 
-          if (is_optional || maybe_none) {
+          if (is_optional && !TypeManager::is_pointer(orig_type)) {
+            extra_convert
+              << "} else {\n"
+              << "  " << param_name << "_local" << default_expr << ";\n"
+              << "  " << param_name << "_this = &" << param_name << "_local;\n"
+              << "}\n";
+          } else if (is_optional || maybe_none) {
             extra_convert << "}\n";
           }
 
@@ -5645,21 +5606,60 @@ write_function_instance(ostream &out, FunctionRemap *remap,
       } else { // The regular, non-coercion case.
         type->output_instance(extra_convert, param_name + "_this", &parser);
         if (is_optional && maybe_none) {
+          // This parameter has a default value of nullptr, so we need to also
+          // allow passing in None.
           extra_convert
             << default_expr << ";\n"
             << "if (" << param_name << " != nullptr && " << param_name << " != Py_None) {\n"
             << "  " << param_name << "_this";
-        } else if (is_optional) {
+        }
+        else if (is_optional && !TypeManager::is_pointer(orig_type) && !default_value->is_lvalue()) {
+          // Most annoying case, where we have to use an rvalue reference to
+          // extend the lifetime of the default argument.  In this case, the
+          // default expression is invoked even if not used.
+          extra_convert << ";\n";
+          if (TypeManager::is_const_pointer_to_anything(type)) {
+            extra_convert << "const ";
+            obj_type->output_instance(extra_convert, "&" + param_name + "_ref", &parser);
+          } else {
+            obj_type->output_instance(extra_convert, "&&" + param_name + "_ref", &parser);
+          }
           extra_convert
             << default_expr << ";\n"
-            << "if (" << param_name << " != nullptr) {\n"
+            << "if (" << param_name << " == nullptr) {\n"
+            << "  " << param_name << "_this = &" << param_name << "_ref;\n"
+            << "} else {\n"
             << "  " << param_name << "_this";
-        } else if (maybe_none) {
+        }
+        else if (is_optional) {
+          // General case where the default argument is either an lvalue or a
+          // pointer.
+          extra_convert
+            << ";\n"
+            << "if (" << param_name << " == nullptr) {\n"
+            << "  " << param_name << "_this = ";
+          if (TypeManager::is_pointer(orig_type)) {
+            default_value->output(extra_convert, 0, &parser, false);
+            extra_convert << ";\n";
+          } else {
+            // The rvalue case was handled above, so this is an lvalue, which
+            // means we can safely take a reference to it.
+            extra_convert << "&(";
+            default_value->output(extra_convert, 0, &parser, false);
+            extra_convert << ");\n";
+          }
+          extra_convert
+            << "} else {\n"
+            << "  " << param_name << "_this";
+        }
+        else if (maybe_none) {
+          // No default argument, but we still need to check for None.
           extra_convert
             << " = nullptr;\n"
             << "if (" << param_name << " != Py_None) {\n"
             << "  " << param_name << "_this";
         }
+
         if (const_ok && !report_errors) {
           // This function does the same thing in this case and is slightly
           // simpler.  But maybe we should just reorganize these functions