From a9d6d3be46be68576008577f1ca1c453b23cddd7 Mon Sep 17 00:00:00 2001 From: rdb Date: Wed, 26 Dec 2018 20:03:14 +0100 Subject: [PATCH] interrogate: refactor default argument handling --- dtool/src/cppparser/cppExpression.cxx | 60 +++- dtool/src/cppparser/cppExpression.h | 1 + .../interfaceMakerPythonNative.cxx | 322 +++++++++--------- 3 files changed, 220 insertions(+), 163 deletions(-) diff --git a/dtool/src/cppparser/cppExpression.cxx b/dtool/src/cppparser/cppExpression.cxx index 2b2cc0bd50..78da68b5a7 100644 --- a/dtool/src/cppparser/cppExpression.cxx +++ b/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 diff --git a/dtool/src/cppparser/cppExpression.h b/dtool/src/cppparser/cppExpression.h index 4482727f53..3cea871679 100644 --- a/dtool/src/cppparser/cppExpression.h +++ b/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; diff --git a/dtool/src/interrogate/interfaceMakerPythonNative.cxx b/dtool/src/interrogate/interfaceMakerPythonNative.cxx index 59575668d4..3964cad919 100644 --- a/dtool/src/interrogate/interfaceMakerPythonNative.cxx +++ b/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 > &map_sets, @@ -4118,70 +4106,8 @@ collapse_default_remaps(std::map > &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::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 > &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 >::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,23 +4703,40 @@ 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; - extra_convert - << "#if PY_VERSION_HEX >= 0x03030000\n" - << "wchar_t *" << param_name << "_str = PyUnicode_AsWideCharString(" << param_name << ", nullptr);\n" - << "#else" - << "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" - << param_name << "_str[" << param_name << "_len] = 0;\n" - << "#endif\n"; + 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" + << "#else" + << "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" + << param_name << "_str[" << param_name << "_len] = 0;\n" + << "#endif\n"; + } pexpr_string = param_name + "_str"; @@ -4803,56 +4747,48 @@ 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; - 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" - << "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"; + 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" + << "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"; - 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"; + 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