From bf93d869a709393797d5c75e1341dfe5b5a60d14 Mon Sep 17 00:00:00 2001 From: Friedemann Kleint Date: Thu, 28 Mar 2019 10:34:45 +0100 Subject: 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 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 Reviewed-by: Cristian Maureira-Fredes --- .../shiboken2/ApiExtractor/abstractmetabuilder.cpp | 101 ++++++++++----------- .../shiboken2/ApiExtractor/abstractmetabuilder.h | 6 +- .../shiboken2/ApiExtractor/abstractmetabuilder_p.h | 2 +- sources/shiboken2/ApiExtractor/apiextractor.cpp | 2 +- sources/shiboken2/ApiExtractor/dependency.h | 10 +- .../shiboken2/generator/shiboken2/cppgenerator.cpp | 13 +-- 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; + +static ClassIndexHash::ConstIterator findByTypeEntry(const ClassIndexHash &map, + const TypeEntry *typeEntry) { - QHash map; - QHash 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 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 hash; - QHash::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 #include +#include + // Dependencies for topologically sorting classes + +class AbstractMetaClass; + struct Dependency { - QString parent; - QString child; + AbstractMetaClass *parent; + AbstractMetaClass *child; }; typedef QVector 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); -- cgit v1.2.3