VitaNuo updated this revision to Diff 556004. VitaNuo marked 5 inline comments as done. VitaNuo added a comment.
Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156659/new/ https://reviews.llvm.org/D156659 Files: clang-tools-extra/clangd/index/SymbolCollector.cpp clang-tools-extra/clangd/index/SymbolCollector.h clang-tools-extra/clangd/unittests/FileIndexTests.cpp clang-tools-extra/clangd/unittests/IndexActionTests.cpp clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
Index: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp =================================================================== --- clang-tools-extra/include-cleaner/lib/FindHeaders.cpp +++ clang-tools-extra/include-cleaner/lib/FindHeaders.cpp @@ -125,7 +125,7 @@ if (FD->getNumParams() == 1) // move(T&& t) return tooling::stdlib::Header::named("<utility>"); - if (FD->getNumParams() == 3) + if (FD->getNumParams() == 3 || FD->getNumParams() == 4) // move(InputIt first, InputIt last, OutputIt dest); return tooling::stdlib::Header::named("<algorithm>"); } else if (FName == "remove") { Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp @@ -14,6 +14,7 @@ #include "index/SymbolCollector.h" #include "clang/Basic/FileManager.h" #include "clang/Basic/FileSystemOptions.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Index/IndexingAction.h" #include "clang/Index/IndexingOptions.h" @@ -1554,6 +1555,8 @@ // Move overloads have special handling. template <typename _T> T&& move(_T&& __value); template <typename _I, typename _O> _O move(_I, _I, _O); + template <typename _T, typename _O, typename _I> _O move( + _T&&, _O, _O, _I); } )cpp", /*Main=*/""); @@ -1565,7 +1568,8 @@ includeHeader("<string>")), // Parameter names are demangled. AllOf(labeled("move(T &&value)"), includeHeader("<utility>")), - AllOf(labeled("move(I, I, O)"), includeHeader("<algorithm>")))); + AllOf(labeled("move(I, I, O)"), includeHeader("<algorithm>")), + AllOf(labeled("move(T &&, O, O, I)"), includeHeader("<algorithm>")))); } TEST_F(SymbolCollectorTest, IWYUPragma) { @@ -1592,7 +1596,7 @@ includeHeader("\"the/good/header.h\"")))); } -TEST_F(SymbolCollectorTest, DISABLED_IWYUPragmaExport) { +TEST_F(SymbolCollectorTest, IWYUPragmaExport) { CollectorOpts.CollectIncludePath = true; const std::string Header = R"cpp(#pragma once #include "exporter.h" @@ -1978,6 +1982,24 @@ qName("A"), hasKind(clang::index::SymbolKind::Concept)))); } +TEST_F(SymbolCollectorTest, IncludeHeaderForwardDecls) { + CollectorOpts.CollectIncludePath = true; + const std::string Header = R"cpp(#pragma once +struct Foo; +#include "full.h" +)cpp"; + auto FullFile = testPath("full.h"); + InMemoryFileSystem->addFile(FullFile, 0, + llvm::MemoryBuffer::getMemBuffer(R"cpp( +#pragma once +struct Foo {};)cpp")); + runSymbolCollector(Header, /*Main=*/"", + /*ExtraArgs=*/{"-I", testRoot()}); + EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf( + qName("Foo"), + includeHeader(URI::create(FullFile).toString())))) + << *Symbols.begin(); +} } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/IndexActionTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/IndexActionTests.cpp +++ clang-tools-extra/clangd/unittests/IndexActionTests.cpp @@ -8,11 +8,15 @@ #include "Headers.h" #include "TestFS.h" +#include "URI.h" #include "index/IndexAction.h" #include "index/Serialization.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" #include "clang/Tooling/Tooling.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include <string> namespace clang { namespace clangd { @@ -40,6 +44,11 @@ return toUri(Path) == URI; } +MATCHER_P(includeHeader, P, "") { + return (arg.IncludeHeaders.size() == 1) && + (arg.IncludeHeaders.begin()->IncludeHeader == P); +} + ::testing::Matcher<const IncludeGraphNode &> includesAre(const std::vector<std::string> &Includes) { return ::testing::Field(&IncludeGraphNode::DirectIncludes, @@ -312,6 +321,26 @@ EXPECT_THAT(*IndexFile.Symbols, testing::Contains(hasName("Bar"))); EXPECT_THAT(*IndexFile.Symbols, Not(testing::Contains(hasName("Baz")))); } + +TEST_F(IndexActionTest, SymbolFromCC) { + std::string MainFilePath = testPath("main.cpp"); + addFile(MainFilePath, R"cpp( + #include "main.h" + void foo() {} + )cpp"); + addFile(testPath("main.h"), R"cpp( + #pragma once + void foo(); + )cpp"); + Opts.FileFilter = [](const SourceManager &SM, FileID F) { + return !SM.getFileEntryRefForID(F)->getName().endswith("main.h"); + }; + IndexFileIn IndexFile = runIndexingAction(MainFilePath, {"-std=c++14"}); + EXPECT_THAT(*IndexFile.Symbols, + UnorderedElementsAre(AllOf( + hasName("foo"), + includeHeader(URI::create(testPath("main.h")).toString())))); +} } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/FileIndexTests.cpp +++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp @@ -239,7 +239,7 @@ "<the/good/header.h>"); } -TEST(FileIndexTest, DISABLED_IWYUPragmaExport) { +TEST(FileIndexTest, IWYUPragmaExport) { FileIndex M; TestTU File; Index: clang-tools-extra/clangd/index/SymbolCollector.h =================================================================== --- clang-tools-extra/clangd/index/SymbolCollector.h +++ clang-tools-extra/clangd/index/SymbolCollector.h @@ -175,6 +175,10 @@ void setIncludeLocation(const Symbol &S, SourceLocation, const include_cleaner::Symbol &Sym); + // Providers for Symbol.IncludeHeaders. + // The final spelling is calculated in finish(). + llvm::DenseMap<SymbolID, std::optional<include_cleaner::Header>> + SymbolProviders; // Files which contain ObjC symbols. // This is finalized and used in finish(). llvm::DenseSet<FileID> FilesWithObjCConstructs; Index: clang-tools-extra/clangd/index/SymbolCollector.cpp =================================================================== --- clang-tools-extra/clangd/index/SymbolCollector.cpp +++ clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -36,7 +36,6 @@ #include "clang/Lex/Preprocessor.h" #include "clang/Lex/Token.h" #include "clang/Tooling/Inclusions/HeaderAnalysis.h" -#include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/SmallVector.h" @@ -821,25 +820,26 @@ // Use the expansion location to get the #include header since this is // where the symbol is exposed. IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(DefLoc).first; -} -llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) { - tooling::stdlib::Lang Lang = tooling::stdlib::Lang::CXX; - if (LangOpts.C11) - Lang = tooling::stdlib::Lang::C; - else if(!LangOpts.CPlusPlus) - return ""; + // We update providers for a symbol with each occurence, as SymbolCollector + // might run while parsing, rather than at the end of a translation unit. + // Hence we see more and more redecls over time. + auto [It, Inserted] = SymbolProviders.try_emplace(S.ID); + auto Headers = + include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes); + if (Headers.empty()) + return; - if (S->Scope == "std::" && S->Name == "move") { - if (!S->Signature.contains(',')) - return "<utility>"; - return "<algorithm>"; - } - - if (auto StdSym = tooling::stdlib::Symbol::named(S->Scope, S->Name, Lang)) - if (auto Header = StdSym->header()) - return Header->name(); - return ""; + auto *HeadersIter = Headers.begin(); + include_cleaner::Header H = *HeadersIter; + while (HeadersIter != Headers.end() && + H.kind() == include_cleaner::Header::Physical && + !tooling::isSelfContainedHeader(H.physical(), SM, + PP->getHeaderSearchInfo())) { + H = *HeadersIter; + HeadersIter++; + } + It->second = H; } void SymbolCollector::finish() { @@ -865,13 +865,16 @@ } } llvm::DenseMap<FileID, bool> FileToContainsImportsOrObjC; + llvm::DenseMap<include_cleaner::Header, std::string> HeaderSpelling; // Fill in IncludeHeaders. // We delay this until end of TU so header guards are all resolved. - for (const auto &[SID, FID] : IncludeFiles) { + for (const auto &[SID, OptionalProvider] : SymbolProviders) { const Symbol *S = Symbols.find(SID); if (!S) continue; + assert(IncludeFiles.find(SID) != IncludeFiles.end()); + const auto FID = IncludeFiles.at(SID); // Determine if the FID is #include'd or #import'ed. Symbol::IncludeDirective Directives = Symbol::Invalid; auto CollectDirectives = shouldCollectIncludePath(S->SymInfo.Kind); @@ -891,20 +894,58 @@ if (Directives == Symbol::Invalid) continue; - // FIXME: Use the include-cleaner library instead. - llvm::StringRef IncludeHeader = getStdHeader(S, ASTCtx->getLangOpts()); - if (IncludeHeader.empty()) - IncludeHeader = HeaderFileURIs->getIncludeHeader(FID); + // Use the include location-based logic for Objective-C symbols. + if (Directives & Symbol::Import) { + if (auto IncludeHeader = HeaderFileURIs->getIncludeHeader(FID); + !IncludeHeader.empty()) { + auto NewSym = *S; + NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives}); + Symbols.insert(NewSym); + } + // FIXME: use providers from include-cleaner library once it's polished + // for Objective-C. + continue; + } + + assert(Directives == Symbol::Include); + // For #include's, use the providers computed by the include-cleaner + // library. + if (!OptionalProvider) + continue; + const auto &H = *OptionalProvider; + const auto [SpellingIt, Inserted] = HeaderSpelling.try_emplace(H); + if (Inserted) { + auto &SM = ASTCtx->getSourceManager(); + if (H.kind() == include_cleaner::Header::Kind::Physical) { + // FIXME: Get rid of this once include-cleaner has support for system + // headers. + if (auto Canonical = + HeaderFileURIs->mapCanonical(H.physical()->getName()); + !Canonical.empty()) + SpellingIt->second = Canonical; + // For physical files, prefer URIs as spellings might change + // depending on the translation unit. + else if (tooling::isSelfContainedHeader(H.physical(), SM, + PP->getHeaderSearchInfo())) + SpellingIt->second = + HeaderFileURIs->toURI(H.physical()->getLastRef()); + } else { + SpellingIt->second = include_cleaner::spellHeader( + {H, PP->getHeaderSearchInfo(), + SM.getFileEntryForID(SM.getMainFileID())}); + } + } - if (!IncludeHeader.empty()) { + if (!SpellingIt->second.empty()) { auto NewSym = *S; - NewSym.IncludeHeaders.push_back({IncludeHeader, 1, Directives}); + NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives}); Symbols.insert(NewSym); } } ReferencedSymbols.clear(); IncludeFiles.clear(); + SymbolProviders.clear(); FilesWithObjCConstructs.clear(); }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits