From kde-commits Thu Jun 30 23:50:46 2016 From: Kevin Funk Date: Thu, 30 Jun 2016 23:50:46 +0000 To: kde-commits Subject: [kdevelop/assistantpopup-ng] languages/clang: Clang: Reduce matches for forward decl assistant Message-Id: X-MARC-Message: https://marc.info/?l=kde-commits&m=146733066312187 Git commit 84017d4a91f0369f561ebe817e72a19b64e1deab by Kevin Funk. Committed on 30/06/2016 at 23:30. Pushed by kfunk into branch 'assistantpopup-ng'. Clang: Reduce matches for forward decl assistant Only propose forward declarations for known type names. This avoids excessive popups of the 'forward declare' assistant. Adapt & simplify test code CCMAIL: apol@kde.org M +109 -95 languages/clang/duchain/unknowndeclarationproblem.cpp M +7 -7 languages/clang/tests/test_assistants.cpp http://commits.kde.org/kdevelop/84017d4a91f0369f561ebe817e72a19b64e1deab diff --git a/languages/clang/duchain/unknowndeclarationproblem.cpp b/langua= ges/clang/duchain/unknowndeclarationproblem.cpp index fee1e41..79486f3 100644 --- a/languages/clang/duchain/unknowndeclarationproblem.cpp +++ b/languages/clang/duchain/unknowndeclarationproblem.cpp @@ -212,7 +212,28 @@ KDevelop::DocumentRange forwardDeclarationPosition(con= st QualifiedIdentifier& id return {IndexedString(source.pathOrUrl()), {line, 0, line, 0}}; } = -QVector possibleDeclarations( const Qualifi= edIdentifier& identifier, const KDevelop::Path& file, const KDevelop::Curso= rInRevision& cursor ) +/** + * Iteratively build all levels of the current scope. A (missing) type any= where + * can be aribtrarily namespaced, so we create the permutations of possible + * nestings of namespaces it can currently be in, + * + * TODO: add detection of namespace aliases, such as 'using namespace KDev= elop;' + * + * namespace foo { + * namespace bar { + * function baz() { + * type var; + * } + * } + * } + * + * Would give: + * foo::bar::baz::type + * foo::bar::type + * foo::type + * type + */ +QVector findPossibleQualifiedIdentifiers( c= onst QualifiedIdentifier& identifier, const KDevelop::Path& file, const KDe= velop::CursorInRevision& cursor ) { DUChainReadLocker lock; const TopDUContext* top =3D DUChainUtils::standardContextForUrl( file.= toUrl() ); @@ -229,29 +250,6 @@ QVector possibleDeclara= tions( const QualifiedIden } = QVector declarations{ identifier }; - auto scopes =3D context->scopeIdentifier(); - - /* - * Iteratively build all levels of the current scope. A (missing) type= anywhere - * can be aribtrarily namespaced, so we create the permutations of pos= sible - * nestings of namespaces it can currently be in, - * - * TODO: add detection of namespace aliases, such as 'using namespace = KDevelop;' - * - * namespace foo { - * namespace bar { - * function baz() { - * type var; - * } - * } - * } - * - * Would give: - * foo::bar::baz::type - * foo::bar::type - * foo::type - * type - */ for( auto scopes =3D context->scopeIdentifier(); !scopes.isEmpty(); sc= opes.pop() ) { declarations.append( scopes + identifier ); } @@ -260,71 +258,50 @@ QVector possibleDeclar= ations( const QualifiedIden return declarations; } = -QStringList duchainCandidates( const QualifiedIdentifier& identifier, cons= t KDevelop::Path& file, const KDevelop::CursorInRevision& cursor ) +QStringList findMatchingIncludeFiles(const QVector declarati= ons) { - DUChainReadLocker lock; - /* - * Search the persistent symbol table for the declaration. If it is kn= own from before, - * determine which file it came from and suggest that - */ QStringList candidates; - for( const auto& declaration : possibleDeclarations( identifier, file,= cursor ) ) { - clangDebug() << "Considering candidate declaration" << declaration; - const IndexedDeclaration* declarations; - uint declarationCount; - PersistentSymbolTable::self().declarations( declaration , declarat= ionCount, declarations ); + for (const auto decl: declarations) { + // skip declarations that don't belong to us + const auto& file =3D decl->topContext()->parsingEnvironmentFile(); + if (!file || file->language() !=3D ParseSession::languageString())= { + continue; + } = - for( uint i =3D 0; i < declarationCount; ++i ) { - auto* decl =3D declarations[ i ].declaration(); + if( dynamic_cast( decl ) ) { + continue; + } = - /* Skip if the declaration is invalid or if it is an alias dec= laration - - * we want the actual declaration (and its file) - */ - if( !decl ) { - continue; - } + if( decl->isForwardDeclaration() ) { + continue; + } = - // skip declarations that don't belong to us - const auto& file =3D decl->topContext()->parsingEnvironmentFil= e(); - if (!file || file->language() !=3D ParseSession::languageStrin= g()) { - continue; - } + const auto filepath =3D decl->url().toUrl().toLocalFile(); = - if( dynamic_cast( decl ) ) { + if( !isBlacklisted( filepath ) ) { + candidates << filepath; + clangDebug() << "Adding" << filepath << "determined from candi= date" << decl->toString(); + } + + for( const auto importer : file->importers() ) { + if( importer->imports().count() !=3D 1 && !isBlacklisted( file= path ) ) { continue; } - - if( decl->isForwardDeclaration() ) { + if( importer->topContext()->localDeclarations().count() ) { continue; } = - const auto filepath =3D decl->url().toUrl().toLocalFile(); - - if( !isBlacklisted( filepath ) ) { - candidates << filepath; - clangDebug() << "Adding" << filepath << "determined from c= andidate" << declaration; + const auto filePath =3D importer->url().toUrl().toLocalFile(); + if( isBlacklisted( filePath ) ) { + continue; } = - for( const auto importer : file->importers() ) { - if( importer->imports().count() !=3D 1 && !isBlacklisted( = filepath ) ) { - continue; - } - if( importer->topContext()->localDeclarations().count() ) { - continue; - } - - const auto filePath =3D importer->url().toUrl().toLocalFil= e(); - if( isBlacklisted( filePath ) ) { - continue; - } - - /* This file is a forwarder, such as - * does not actually implement the functions, but= include other headers that do - * we prefer this to other headers - */ - candidates << filePath; - clangDebug() << "Adding forwarder file" << filePath << "to= the result set"; - } + /* This file is a forwarder, such as + * does not actually implement the functions, but inc= lude other headers that do + * we prefer this to other headers + */ + candidates << filePath; + clangDebug() << "Adding forwarder file" << filePath << "to the= result set"; } } = @@ -395,51 +372,88 @@ KDevelop::Path::List includePaths( const KDevelop::Pa= th& file ) /** * Return a list of header files viable for inclusions. All elements will = be unique */ -QStringList includeFiles( const QualifiedIdentifier& identifier, const KDe= velop::Path& file, const KDevelop::DocumentRange& range ) +QStringList includeFiles(const QualifiedIdentifier& identifier, const QVec= tor declarations, const KDevelop::Path& file) { - const CursorInRevision cursor{ range.start().line(), range.start().col= umn() }; const auto includes =3D includePaths( file ); - if( includes.isEmpty() ) { clangDebug() << "Include path is empty"; return {}; } = - const auto candidates =3D duchainCandidates( identifier, file, cursor = ); + const auto candidates =3D findMatchingIncludeFiles(declarations); if( !candidates.isEmpty() ) { // If we find a candidate from the duchain we don't bother scannin= g the include paths return candidates; } = - return scanIncludePaths( identifier, includes ); + return scanIncludePaths(identifier, includes); } = /** * Construct viable forward declarations for the type name. - * - * Currently we're not able to determine what is namespaces, class names e= tc - * and makes a suitable forward declaration, so just suggest "vanilla" dec= larations. */ -ClangFixits forwardDeclarations(const QualifiedIdentifier& identifier, con= st Path& source) +ClangFixits forwardDeclarations(const QVector& matchingDecla= rations, const Path& source) { - const auto range =3D forwardDeclarationPosition(identifier, source); - if (!range.isValid()) { - return {}; + ClangFixits fixits; + for (const auto decl : matchingDeclarations) { + const auto qid =3D decl->qualifiedIdentifier(); + if (qid.count() > 1) { + // TODO: Currently we're not able to determine what is namespa= ces, class names etc + // and makes a suitable forward declaration, so just suggest "= vanilla" declarations. + continue; + } + + const auto range =3D forwardDeclarationPosition(qid, source); + if (!range.isValid()) { + continue; // do not know where to insert + } + + const auto name =3D qid.last().toString(); + fixits +=3D { + {QLatin1String("class ") + name + QLatin1String(";\n"), range,= QObject::tr("Forward declare as 'class'")}, + {QLatin1String("struct ") + name + QLatin1String(";\n"), range= , QObject::tr("Forward declare as 'struct'")} + }; } + return fixits; +} = - const auto name =3D identifier.last().toString(); - return { - {QLatin1String("class ") + name + QLatin1String(";\n"), range, QOb= ject::tr("Forward declare as 'class'")}, - {QLatin1String("struct ") + name + QLatin1String(";\n"), range, QO= bject::tr("Forward declare as 'struct'")} - }; +/** + * Search the persistent symbol table for matching declarations for identi= fiers @p identifiers + */ +QVector findMatchingDeclarations(const QVector& identifiers) +{ + DUChainReadLocker lock; + + QVector matchingDeclarations; + matchingDeclarations.reserve(identifiers.size()); + for (const auto& declaration : identifiers) { + clangDebug() << "Considering candidate declaration" << declaration; + const IndexedDeclaration* declarations; + uint declarationCount; + PersistentSymbolTable::self().declarations( declaration , declarat= ionCount, declarations ); + + for (uint i =3D 0; i < declarationCount; ++i) { + // Skip if the declaration is invalid or if it is an alias dec= laration - + // we want the actual declaration (and its file) + if (auto decl =3D declarations[i].declaration()) { + matchingDeclarations << decl; + } + } + } + return matchingDeclarations; } = ClangFixits fixUnknownDeclaration( const QualifiedIdentifier& identifier, = const KDevelop::Path& file, const KDevelop::DocumentRange& docrange ) { ClangFixits fixits; = + const CursorInRevision cursor{docrange.start().line(), docrange.start(= ).column()}; + + const auto possibleIdentifiers =3D findPossibleQualifiedIdentifiers(id= entifier, file, cursor); + const auto matchingDeclarations =3D findMatchingDeclarations(possibleI= dentifiers); + if (ClangSettingsManager::self()->assistantsSettings().forwardDeclare)= { - for (const auto& fixit : forwardDeclarations(identifier, file)) { + for (const auto& fixit : forwardDeclarations(matchingDeclarations,= file)) { fixits << fixit; if (fixits.size() =3D=3D maxSuggestions) { return fixits; @@ -447,7 +461,7 @@ ClangFixits fixUnknownDeclaration( const QualifiedIdent= ifier& identifier, const } } = - const auto includefiles =3D includeFiles( identifier, file, docrange ); + const auto includefiles =3D includeFiles(identifier, matchingDeclarati= ons, file); if (includefiles.isEmpty()) { // return early as the computation of the include paths is quite e= xpensive return fixits; diff --git a/languages/clang/tests/test_assistants.cpp b/languages/clang/te= sts/test_assistants.cpp index 5b4d60f..0302257 100644 --- a/languages/clang/tests/test_assistants.cpp +++ b/languages/clang/tests/test_assistants.cpp @@ -545,13 +545,13 @@ void TestAssistants::testSignatureAssistant() QCOMPARE(testbed.documentText(Testbed::CppDoc), finalCppContents); } = -enum UnknownDeclarationActions +enum UnknownDeclarationAction { - NoUnknownDeclaration =3D 0x0, + NoUnknownDeclarationAction =3D 0x0, ForwardDecls =3D 0x1, MissingInclude =3D 0x2 }; - +Q_DECLARE_FLAGS(UnknownDeclarationActions, UnknownDeclarationAction) Q_DECLARE_METATYPE(UnknownDeclarationActions) = void TestAssistants::testUnknownDeclarationAssistant_data() @@ -562,11 +562,11 @@ void TestAssistants::testUnknownDeclarationAssistant_= data() QTest::addColumn("actions"); = QTest::newRow("unincluded_struct") << "struct test{};" << "" << "test" - << static_cast(ForwardDecls | MissingIn= clude); + << UnknownDeclarationActions(ForwardDecls | MissingInclude); QTest::newRow("forward_declared_struct") << "struct test{};" << "struc= t test;" << "test *f; f->" - << static_cast(MissingInclude); + << UnknownDeclarationActions(MissingInclude); QTest::newRow("unknown_struct") << "" << "" << "test" - << static_cast(ForwardDecls); + << UnknownDeclarationActions(); } = void TestAssistants::testUnknownDeclarationAssistant() @@ -590,7 +590,7 @@ void TestAssistants::testUnknownDeclarationAssistant() = const auto problems =3D topCtx->problems(); = - if (actions =3D=3D NoUnknownDeclaration) { + if (actions =3D=3D NoUnknownDeclarationAction) { QVERIFY(!problems.isEmpty()); return; }