shiboken6: Refactor argument conversion for virtual method overrides

Virtual method overrides use Py_BuildValue() to create the argument
tuple for the call with its built-in conversions.

Fold the code for creating the argument and its format string
into a new helper CppGenerator::virtualMethodNativeArg().
Remove ShibokenGenerator::getFormatUnitString().

Change the logic to check for conversion rules from modifications and
convertable primitives first and fall back to shiboken conversions for
the rest, removing the complicated condition duplicated in
ShibokenGenerator::getFormatUnitString() and
CppGenerator::writeVirtualMethodNativeArgs().

This fixes a bug where no conversion was created when passing
smart pointer by values in a virtual functions.

Task-number: PYSIDE-454
Change-Id: I20ef047c89a0d0047f5234a90aae548ca8e6b932
Reviewed-by: Christian Tismer <tismer@stackless.com>

4 files changed, 53 insertions, 91 deletions

diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.cpp b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
index c2e6a140e..de0f69e7f 100644
--- a/sources/shiboken6/generator/shiboken/cppgenerator.cpp
+++ b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
@@ -1143,6 +1143,30 @@ QString CppGenerator::virtualMethodReturn(TextStream &s, const ApiExtractorResul
            + u';';
 }
 
+// Create an argument for Py_BuildValue() when writing virtual methods.
+// Return a pair of (argument, format-char).
+QPair<QString, QChar> CppGenerator::virtualMethodNativeArg(const AbstractMetaFunctionCPtr &func,
+                                                             const AbstractMetaArgument &arg)
+{
+    if (func->hasConversionRule(TypeSystem::TargetLangCode, arg.argumentIndex() + 1))
+        return {arg.name() + CONV_RULE_OUT_VAR_SUFFIX, u'N'};
+
+    const auto &type = arg.type();
+    auto *argTypeEntry = type.typeEntry();
+    // Check for primitive types convertible by Py_BuildValue()
+    if (argTypeEntry->isPrimitive() && !type.isCString()) {
+        const auto *pte = argTypeEntry->asPrimitive()->basicReferencedTypeEntry();
+        auto it = formatUnits().constFind(pte->name());
+        if (it != formatUnits().constEnd())
+            return {arg.name(), it.value()};
+    }
+
+    // Rest: convert
+    StringStream ac(TextStream::Language::Cpp);
+    writeToPythonConversion(ac, type, func->ownerClass(), arg.name());
+    return {ac.toString(), u'N'};
+}
+
 void CppGenerator::writeVirtualMethodNativeArgs(TextStream &s,
                                                 const AbstractMetaFunctionCPtr &func,
                                                 const AbstractMetaArgumentList &arguments,
@@ -1153,36 +1177,16 @@ void CppGenerator::writeVirtualMethodNativeArgs(TextStream &s,
         s << "PyTuple_New(0));\n";
         return;
     }
+
+    QString format;
     QStringList argConversions;
     for (const AbstractMetaArgument &arg : arguments) {
-        const auto &argType = arg.type();
-        const auto *argTypeEntry = argType.typeEntry();
-        bool convert = argTypeEntry->isObject()
-                        || argTypeEntry->isValue()
-                        || argType.isValuePointer()
-                        || argType.isNativePointer()
-                        || argTypeEntry->isFlags()
-                        || argTypeEntry->isEnum()
-                        || argTypeEntry->isContainer()
-                        || argType.referenceType() == LValueReference;
-        if (!convert && argTypeEntry->isPrimitive()) {
-            const auto *pte = argTypeEntry->asPrimitive()->basicReferencedTypeEntry();
-            convert = !formatUnits().contains(pte->name());
-        }
-        StringStream ac(TextStream::Language::Cpp);
-        if (func->hasConversionRule(TypeSystem::TargetLangCode,
-                                    arg.argumentIndex() + 1)) {
-            ac << arg.name() + CONV_RULE_OUT_VAR_SUFFIX;
-        } else {
-            QString argName = arg.name();
-            if (convert)
-                writeToPythonConversion(ac, arg.type(), func->ownerClass(), argName);
-            else
-                ac << argName;
-        }
-        argConversions << ac.toString();
+        auto argPair = virtualMethodNativeArg(func, arg);
+        argConversions.append(argPair.first);
+        format += argPair.second;
     }
-    s << "Py_BuildValue(\"(" << getFormatUnitString(func, false) << ")\",\n"
+
+    s << "Py_BuildValue(\"(" << format << ")\",\n"
         << indent << argConversions.join(u",\n"_s) << outdent << "\n));\n";
 
     for (int index : qAsConst(invalidateArgs)) {
diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.h b/sources/shiboken6/generator/shiboken/cppgenerator.h
index a9f8cae9e..a989fe35f 100644
--- a/sources/shiboken6/generator/shiboken/cppgenerator.h
+++ b/sources/shiboken6/generator/shiboken/cppgenerator.h
@@ -69,6 +69,9 @@ private:
     void writeDestructorNative(TextStream &s, const GeneratorContext &classContext) const;
 
     QString getVirtualFunctionReturnTypeName(const AbstractMetaFunctionCPtr &func) const;
+    static QPair<QString, QChar>
+        virtualMethodNativeArg(const AbstractMetaFunctionCPtr &func,
+                               const AbstractMetaArgument &arg);
     void writeVirtualMethodNativeArgs(TextStream &s,
                                       const AbstractMetaFunctionCPtr &func,
                                       const AbstractMetaArgumentList &arguments,
diff --git a/sources/shiboken6/generator/shiboken/shibokengenerator.cpp b/sources/shiboken6/generator/shiboken/shibokengenerator.cpp
index e583ed4b3..e8562a254 100644
--- a/sources/shiboken6/generator/shiboken/shibokengenerator.cpp
+++ b/sources/shiboken6/generator/shiboken/shibokengenerator.cpp
@@ -135,24 +135,23 @@ static const QHash<QString, QString> &primitiveTypesCorrespondences()
     return result;
 }
 
-// Format units for C++->Python->C++ conversion
-const QHash<QString, QString> &ShibokenGenerator::formatUnits()
-{
-    static const QHash<QString, QString> result = {
-        {u"char"_s, u"b"_s},
-        {u"unsigned char"_s, u"B"_s},
-        {intT(), u"i"_s},
-        {u"unsigned int"_s, u"I"_s},
-        {shortT(), u"h"_s},
-        {unsignedShortT(), u"H"_s},
-        {longT(), u"l"_s},
-        {unsignedLongLongT(), u"k"_s},
-        {longLongT(), u"L"_s},
-        {u"__int64"_s, u"L"_s},
-        {unsignedLongLongT(), u"K"_s},
-        {u"unsigned __int64"_s, u"K"_s},
-        {doubleT(), u"d"_s},
-        {floatT(), u"f"_s},
+const QHash<QString, QChar> &ShibokenGenerator::formatUnits()
+{
+    static const QHash<QString, QChar> result = {
+        {u"char"_s, u'b'},
+        {u"unsigned char"_s, u'B'},
+        {intT(), u'i'},
+        {u"unsigned int"_s, u'I'},
+        {shortT(), u'h'},
+        {unsignedShortT(), u'H'},
+        {longT(), u'l'},
+        {unsignedLongLongT(), u'k'},
+        {longLongT(), u'L'},
+        {u"__int64"_s, u'L'},
+        {unsignedLongLongT(), u'K'},
+        {u"unsigned __int64"_s, u'K'},
+        {doubleT(), u'd'},
+        {floatT(), u'f'},
     };
     return result;
 }
@@ -513,49 +512,6 @@ bool ShibokenGenerator::shouldRejectNullPointerArgument(const AbstractMetaFuncti
     return false;
 }
 
-QString ShibokenGenerator::getFormatUnitString(const AbstractMetaFunctionCPtr &func, bool incRef)
-{
-    QString result;
-    const char objType = (incRef ? 'O' : 'N');
-    const AbstractMetaArgumentList &arguments = func->arguments();
-    for (const AbstractMetaArgument &arg : arguments) {
-        if (arg.isModifiedRemoved())
-            continue;
-
-        const auto &type = arg.type();
-        if (arg.isTypeModified()) {
-            result += QLatin1Char(objType);
-        } else if (arg.type().isObject()
-            || type.isValue()
-            || type.isValuePointer()
-            || type.isNativePointer()
-            || type.isEnum()
-            || type.isFlags()
-            || type.isContainer()
-            || type.isSmartPointer()
-            || type.referenceType() == LValueReference) {
-            result += QLatin1Char(objType);
-        } else if (type.isPrimitive()) {
-            const auto *ptype = type.typeEntry()->asPrimitive()->basicReferencedTypeEntry();
-            const auto it = formatUnits().constFind(ptype->name());
-            if (it != formatUnits().cend())
-                result += it.value();
-            else
-                result += QLatin1Char(objType);
-        } else if (type.isCString()) {
-            result += u'z';
-        } else {
-            qCWarning(lcShiboken).noquote().nospace()
-                << "Method: " << func->ownerClass()->qualifiedCppName()
-                << "::" << func->signature() << " => Arg:"
-                << arg.name() << "index: " << arg.argumentIndex()
-                << " - cannot be handled properly. Use an inject-code to fix it!";
-            result += u'?';
-        }
-    }
-    return result;
-}
-
 QString ShibokenGenerator::cpythonBaseName(const AbstractMetaType &type)
 {
     if (type.isCString())
diff --git a/sources/shiboken6/generator/shiboken/shibokengenerator.h b/sources/shiboken6/generator/shiboken/shibokengenerator.h
index 7fdc10907..6802cf9e6 100644
--- a/sources/shiboken6/generator/shiboken/shibokengenerator.h
+++ b/sources/shiboken6/generator/shiboken/shibokengenerator.h
@@ -252,8 +252,6 @@ protected:
     /// Returns the special cast function name, the function used to proper cast class with multiple inheritance.
     static QString cpythonSpecialCastFunctionName(const AbstractMetaClass *metaClass);
 
-    static QString getFormatUnitString(const AbstractMetaFunctionCPtr &func, bool incRef = false);
-
     /// Returns the file name for the module global header. If no module name is provided the current will be used.
     static QString getModuleHeaderFileName(const QString &moduleName = QString());
     static QString getPrivateModuleHeaderFileName(const QString &moduleName = QString());
@@ -310,7 +308,8 @@ protected:
 
     static QString pythonArgsAt(int i);
 
-    static const QHash<QString, QString> &formatUnits();
+    /// Return the format character for C++->Python->C++ conversion (Py_BuildValue)
+    static const QHash<QString, QChar> &formatUnits();
 
 private:
     static QString getModuleHeaderFileBaseName(const QString &moduleName = QString());