shiboken: Refactor AbstractMetaBuilder::classesTopologicalSorted()

Change the function parameter to be a list always, making the logic of
the inner classes clearer.

In the implementation, use a of QHash<AbstractMetaClass*, int> instead
of hashing by name, which makes it possible to disambiguate namespaces
extended in modules.

This also allows for a drastic simplification of the code trying to
determine the dependency given by parameter default values. Instead of
trying to match by name, correctly qualifying it, the matching can be
done by TypeEntry pointers.

Change-Id: Ia17bf6e109576bac029fb016e5e11309777d0735
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>

6 files changed, 64 insertions, 70 deletions

diff --git a/sources/shiboken2/ApiExtractor/abstractmetabuilder.cpp b/sources/shiboken2/ApiExtractor/abstractmetabuilder.cpp
index 5074770d3..e62a2a78a 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetabuilder.cpp
+++ b/sources/shiboken2/ApiExtractor/abstractmetabuilder.cpp
@@ -669,7 +669,7 @@ void AbstractMetaBuilderPrivate::traverseDom(const FileModelItem &dom)
     checkFunctionModifications();
 
     // sort all classes topologically
-    m_metaClasses = classesTopologicalSorted();
+    m_metaClasses = classesTopologicalSorted(m_metaClasses);
 
     for (AbstractMetaClass* cls : qAsConst(m_metaClasses)) {
 //         setupEquals(cls);
@@ -681,7 +681,7 @@ void AbstractMetaBuilderPrivate::traverseDom(const FileModelItem &dom)
         if (!cls->typeEntry()->codeGeneration() || cls->innerClasses().size() < 2)
             continue;
 
-        cls->setInnerClasses(classesTopologicalSorted(cls));
+        cls->setInnerClasses(classesTopologicalSorted(cls->innerClasses()));
     }
 
     dumpLog();
@@ -3085,26 +3085,37 @@ void AbstractMetaBuilderPrivate::dumpLog() const
     writeRejectLogFile(m_logDirectory + QLatin1String("mjb_rejected_fields.log"), m_rejectedFields);
 }
 
-AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const AbstractMetaClass *cppClass,
-                                                                           const Dependencies &additionalDependencies) const
+using ClassIndexHash = QHash<AbstractMetaClass *, int>;
+
+static ClassIndexHash::ConstIterator findByTypeEntry(const ClassIndexHash &map,
+                                                     const TypeEntry *typeEntry)
 {
-    QHash<QString, int> map;
-    QHash<int, AbstractMetaClass*> reverseMap;
+    auto it = map.cbegin();
+    for (auto end = map.cend(); it != end; ++it) {
+        if (it.key()->typeEntry() == typeEntry)
+            break;
+    }
+    return it;
+}
 
-    const AbstractMetaClassList& classList = cppClass ? cppClass->innerClasses() : m_metaClasses;
+AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const AbstractMetaClassList &classList,
+                                                                           const Dependencies &additionalDependencies) const
+{
+    ClassIndexHash map;
+    QHash<int, AbstractMetaClass *> reverseMap;
 
     int i = 0;
     for (AbstractMetaClass *clazz : classList) {
-        if (map.contains(clazz->qualifiedCppName()))
+        if (map.contains(clazz))
             continue;
-        map[clazz->qualifiedCppName()] = i;
-        reverseMap[i] = clazz;
+        map.insert(clazz, i);
+        reverseMap.insert(i, clazz);
         i++;
     }
 
     Graph graph(map.count());
 
-    for (const Dependency &dep : additionalDependencies) {
+    for (const auto &dep : additionalDependencies) {
         const int parentIndex = map.value(dep.parent, -1);
         const int childIndex = map.value(dep.child, -1);
         if (parentIndex >= 0 && childIndex >= 0) {
@@ -3112,18 +3123,17 @@ AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const
         } else {
             qCWarning(lcShiboken).noquote().nospace()
                 << "AbstractMetaBuilder::classesTopologicalSorted(): Invalid additional dependency: "
-                << dep.child << " -> " << dep.parent << '.';
+                << dep.child->name() << " -> " << dep.parent->name() << '.';
         }
     }
 
-    // TODO choose a better name to these regexs
-    static const QRegularExpression regex1(QStringLiteral("\\(.*\\)"));
-    Q_ASSERT(regex1.isValid());
-    static const QRegularExpression regex2(QStringLiteral("::.*"));
-    Q_ASSERT(regex2.isValid());
     for (AbstractMetaClass *clazz : classList) {
-        if (clazz->enclosingClass() && map.contains(clazz->enclosingClass()->qualifiedCppName()))
-            graph.addEdge(map[clazz->enclosingClass()->qualifiedCppName()], map[clazz->qualifiedCppName()]);
+        const int classIndex = map.value(clazz);
+        if (auto enclosing = clazz->enclosingClass()) {
+            const auto enclosingIt = map.constFind(const_cast< AbstractMetaClass *>(enclosing));
+            if (enclosingIt!= map.cend())
+                graph.addEdge(enclosingIt.value(), classIndex);
+        }
 
         const AbstractMetaClassList &bases = getBaseClasses(clazz);
         for (AbstractMetaClass *baseClass : bases) {
@@ -3131,43 +3141,25 @@ AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const
             if (clazz->baseClass() == baseClass)
                 clazz->setBaseClass(baseClass);
 
-            if (map.contains(baseClass->qualifiedCppName()))
-                graph.addEdge(map[baseClass->qualifiedCppName()], map[clazz->qualifiedCppName()]);
+            const auto baseIt = map.constFind(baseClass);
+            if (baseIt!= map.cend())
+                graph.addEdge(baseIt.value(), classIndex);
         }
 
         const AbstractMetaFunctionList &functions = clazz->functions();
         for (AbstractMetaFunction *func : functions) {
             const AbstractMetaArgumentList &arguments = func->arguments();
             for (AbstractMetaArgument *arg : arguments) {
-                // check methods with default args
-                QString defaultExpression = arg->originalDefaultValueExpression();
-                if (!defaultExpression.isEmpty()) {
-                    if (defaultExpression == QLatin1String("0") && arg->type()->isValue())
-                        defaultExpression = arg->type()->name();
-
-                    defaultExpression.remove(regex1);
-                    defaultExpression.remove(regex2);
-                }
-                if (!defaultExpression.isEmpty()) {
-                    QString exprClassName = clazz->qualifiedCppName() + colonColon() + defaultExpression;
-                    if (!map.contains(exprClassName)) {
-                        bool found = false;
-                        for (AbstractMetaClass *baseClass : bases) {
-                            exprClassName = baseClass->qualifiedCppName() + colonColon() + defaultExpression;
-                            if (map.contains(exprClassName)) {
-                                found = true;
-                                break;
-                            }
-                        }
-                        if (!found) {
-                            if (map.contains(defaultExpression))
-                                exprClassName = defaultExpression;
-                            else
-                                exprClassName.clear();
-                        }
+                // Check methods with default args: If a class is instantiated by value,
+                // ("QString s = QString()"), add a dependency.
+                if (!arg->originalDefaultValueExpression().isEmpty()
+                    && arg->type()->isValue()) {
+                    auto typeEntry = arg->type()->typeEntry();
+                    if (typeEntry->isComplex() && typeEntry != clazz->typeEntry()) {
+                        auto ait = findByTypeEntry(map, typeEntry);
+                            if (ait != map.cend() && ait.key()->enclosingClass() != clazz)
+                                graph.addEdge(ait.value(), classIndex);
                     }
-                    if (!exprClassName.isEmpty() && exprClassName != clazz->qualifiedCppName())
-                        graph.addEdge(map[exprClassName], map[clazz->qualifiedCppName()]);
                 }
             }
         }
@@ -3176,13 +3168,12 @@ AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const
     AbstractMetaClassList result;
     const auto unmappedResult = graph.topologicalSort();
     if (unmappedResult.isEmpty() && graph.nodeCount()) {
-        QTemporaryFile tempFile;
+        QTemporaryFile tempFile(QDir::tempPath() + QLatin1String("/cyclic_depXXXXXX.dot"));
         tempFile.setAutoRemove(false);
         tempFile.open();
         QHash<int, QString> hash;
-        QHash<QString, int>::iterator it = map.begin();
-        for (; it != map.end(); ++it)
-            hash[it.value()] = it.key();
+        for (auto it = map.cbegin(), end = map.cend(); it != end; ++it)
+            hash.insert(it.value(), it.key()->qualifiedCppName());
         graph.dumpDot(hash, tempFile.fileName());
         qCWarning(lcShiboken).noquote().nospace()
             << "Cyclic dependency found! Graph can be found at "
@@ -3198,10 +3189,10 @@ AbstractMetaClassList AbstractMetaBuilderPrivate::classesTopologicalSorted(const
     return result;
 }
 
-AbstractMetaClassList AbstractMetaBuilder::classesTopologicalSorted(const AbstractMetaClass *cppClass,
+AbstractMetaClassList AbstractMetaBuilder::classesTopologicalSorted(const AbstractMetaClassList &classList,
                                                                     const Dependencies &additionalDependencies) const
 {
-    return d->classesTopologicalSorted(cppClass, additionalDependencies);
+    return d->classesTopologicalSorted(classList, additionalDependencies);
 }
 
 AbstractMetaArgumentList AbstractMetaBuilderPrivate::reverseList(const AbstractMetaArgumentList &list)
diff --git a/sources/shiboken2/ApiExtractor/abstractmetabuilder.h b/sources/shiboken2/ApiExtractor/abstractmetabuilder.h
index 7e5c1fc79..ed89060ac 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetabuilder.h
+++ b/sources/shiboken2/ApiExtractor/abstractmetabuilder.h
@@ -68,12 +68,10 @@ public:
     AbstractMetaEnum *findEnum(const TypeEntry *typeEntry) const;
 
     /**
-    *   Sorts a list of classes topologically, if an AbstractMetaClass object
-    *   is passed the list of classes will be its inner classes, otherwise
-    *   the list will be the module global classes.
+    *   Sorts a list of classes topologically.
     *   \return a list of classes sorted topologically
     */
-    AbstractMetaClassList classesTopologicalSorted(const AbstractMetaClass *cppClass = Q_NULLPTR,
+    AbstractMetaClassList classesTopologicalSorted(const AbstractMetaClassList &classList,
                                                    const Dependencies &additionalDependencies = Dependencies()) const;
 
     bool build(const QByteArrayList &arguments,
diff --git a/sources/shiboken2/ApiExtractor/abstractmetabuilder_p.h b/sources/shiboken2/ApiExtractor/abstractmetabuilder_p.h
index 185dd0e30..d8203a586 100644
--- a/sources/shiboken2/ApiExtractor/abstractmetabuilder_p.h
+++ b/sources/shiboken2/ApiExtractor/abstractmetabuilder_p.h
@@ -52,7 +52,7 @@ public:
     void traverseDom(const FileModelItem &dom);
 
     void dumpLog() const;
-    AbstractMetaClassList classesTopologicalSorted(const AbstractMetaClass *cppClass = Q_NULLPTR,
+    AbstractMetaClassList classesTopologicalSorted(const AbstractMetaClassList &classList,
                                                    const Dependencies &additionalDependencies = Dependencies()) const;
     ScopeModelItem popScope() { return m_scopes.takeLast(); }
 
diff --git a/sources/shiboken2/ApiExtractor/apiextractor.cpp b/sources/shiboken2/ApiExtractor/apiextractor.cpp
index 7d2ce250e..e301d891f 100644
--- a/sources/shiboken2/ApiExtractor/apiextractor.cpp
+++ b/sources/shiboken2/ApiExtractor/apiextractor.cpp
@@ -153,7 +153,7 @@ AbstractMetaClassList ApiExtractor::smartPointers() const
 AbstractMetaClassList ApiExtractor::classesTopologicalSorted(const Dependencies &additionalDependencies) const
 {
     Q_ASSERT(m_builder);
-    return m_builder->classesTopologicalSorted(Q_NULLPTR, additionalDependencies);
+    return m_builder->classesTopologicalSorted(m_builder->classes(), additionalDependencies);
 }
 
 PrimitiveTypeEntryList ApiExtractor::primitiveTypes() const
diff --git a/sources/shiboken2/ApiExtractor/dependency.h b/sources/shiboken2/ApiExtractor/dependency.h
index 97ae32df9..d563e9094 100644
--- a/sources/shiboken2/ApiExtractor/dependency.h
+++ b/sources/shiboken2/ApiExtractor/dependency.h
@@ -29,13 +29,17 @@
 #ifndef DEPENDENCY_H
 #define DEPENDENCY_H
 
-#include <QtCore/QString>
 #include <QtCore/QVector>
 
+#include <utility>
+
 // Dependencies for topologically sorting classes
+
+class AbstractMetaClass;
+
 struct Dependency {
-    QString parent;
-    QString child;
+    AbstractMetaClass *parent;
+    AbstractMetaClass *child;
 };
 
 typedef QVector<Dependency> Dependencies;
diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
index b4337c2b1..039a2928b 100644
--- a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
+++ b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp
@@ -5430,12 +5430,13 @@ bool CppGenerator::finishGeneration()
     //We need move QMetaObject register before QObject
     Dependencies additionalDependencies;
     const AbstractMetaClassList &allClasses = classes();
-    if (AbstractMetaClass::findClass(allClasses, qObjectClassName()) != Q_NULLPTR
-        && AbstractMetaClass::findClass(allClasses, qMetaObjectClassName()) != Q_NULLPTR) {
-        Dependency dependency;
-        dependency.parent = qMetaObjectClassName();
-        dependency.child = qObjectClassName();
-        additionalDependencies.append(dependency);
+    if (auto qObjectClass = AbstractMetaClass::findClass(allClasses, qObjectClassName())) {
+        if (auto qMetaObjectClass = AbstractMetaClass::findClass(allClasses, qMetaObjectClassName())) {
+            Dependency dependency;
+            dependency.parent = qMetaObjectClass;
+            dependency.child = qObjectClass;
+            additionalDependencies.append(dependency);
+        }
     }
     const AbstractMetaClassList lst = classesTopologicalSorted(additionalDependencies);