sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: cfe-commits, arphaman, mgrang, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang.
Both of these attempt to check whether a header guard exists while parsing the file. However the file is only marked as guarded once clang finishes processing it. We defer the checks and work until SymbolCollector::finish(). This is ugly and ad-hoc, deferring *all* work might be cleaner. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D61442 Files: clangd/index/Symbol.cpp clangd/index/Symbol.h clangd/index/SymbolCollector.cpp clangd/index/SymbolCollector.h clangd/unittests/SymbolCollectorTests.cpp
Index: clangd/unittests/SymbolCollectorTests.cpp =================================================================== --- clangd/unittests/SymbolCollectorTests.cpp +++ clangd/unittests/SymbolCollectorTests.cpp @@ -20,6 +20,7 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/VirtualFileSystem.h" +#include "gmock/gmock-matchers.h" #include "gmock/gmock-more-matchers.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -34,9 +35,9 @@ using testing::_; using testing::AllOf; using testing::Contains; +using testing::Each; using testing::ElementsAre; using testing::Field; -using testing::IsEmpty; using testing::Not; using testing::Pair; using testing::UnorderedElementsAre; @@ -1028,6 +1029,26 @@ Not(IncludeHeader())))); } +// Features that depend on header-guards are fragile. Header guards are only +// recognized when the file ends, so we have to defer checking for them. +TEST_F(SymbolCollectorTest, HeaderGuardDetected) { + CollectorOpts.CollectIncludePath = true; + CollectorOpts.CollectMacro = true; + runSymbolCollector(R"cpp( + #ifndef HEADER_GUARD_ + #define HEADER_GUARD_ + + // Symbols are seen before the header guard is complete. + int decl(); + #define MACRO + + #endif // Header guard is recognized here. + )cpp", + ""); + EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_")))); + EXPECT_THAT(Symbols, Each(IncludeHeader())); +} + TEST_F(SymbolCollectorTest, NonModularHeader) { auto TU = TestTU::withHeaderCode("int x();"); EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader())); Index: clangd/index/SymbolCollector.h =================================================================== --- clangd/index/SymbolCollector.h +++ clangd/index/SymbolCollector.h @@ -125,6 +125,11 @@ // All Symbols collected from the AST. SymbolSlab::Builder Symbols; + // File IDs for Symbol.IncludeHeaders. + // The final spelling is calculated in finish(). + llvm::DenseMap<SymbolID, FileID> IncludeFiles; + // Indexed macros, to be erased if they turned out to be include guards. + llvm::DenseSet<const IdentifierInfo *> IndexedMacros; // All refs collected from the AST. // Only symbols declared in preamble (from #include) and referenced from the // main file will be included. Index: clangd/index/SymbolCollector.cpp =================================================================== --- clangd/index/SymbolCollector.cpp +++ clangd/index/SymbolCollector.cpp @@ -352,9 +352,8 @@ const auto &SM = PP->getSourceManager(); auto DefLoc = MI->getDefinitionLoc(); - // Header guards are not interesting in index. Builtin macros don't have - // useful locations and are not needed for code completions. - if (MI->isUsedForHeaderGuard() || MI->isBuiltinMacro()) + // Builtin macros don't have useful locations and aren't needed in completion. + if (MI->isBuiltinMacro()) return true; // Skip main-file symbols if we are not collecting them. @@ -409,17 +408,12 @@ std::string SnippetSuffix; getSignature(*CCS, &Signature, &SnippetSuffix); - std::string Include; - if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) { - if (auto Header = getIncludeHeader( - Name->getName(), SM.getDecomposedExpansionLoc(DefLoc).first)) - Include = std::move(*Header); - } + if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) + IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(DefLoc).first; S.Signature = Signature; S.CompletionSnippetSuffix = SnippetSuffix; - if (!Include.empty()) - S.IncludeHeaders.emplace_back(Include, 1); + IndexedMacros.insert(Name); Symbols.insert(S); return true; } @@ -440,6 +434,14 @@ } if (Opts.CollectMacro) { assert(PP); + // First, drop header guards. We can't identify these until EOF. + for (const IdentifierInfo *II : IndexedMacros) { + if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo()) + if (auto ID = getSymbolID(*II, MI, PP->getSourceManager())) + if (MI->isUsedForHeaderGuard()) + Symbols.erase(*ID); + } + // Now increment refcounts. for (const IdentifierInfo *II : ReferencedMacros) { if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo()) if (auto ID = getSymbolID(*II, MI, PP->getSourceManager())) @@ -447,6 +449,21 @@ } } + // Fill in IncludeHeaders. + // We delay this until end of TU so header guards are all resolved. + // Symbols in slabs aren' mutable, so insert() has to walk all the strings :-( + llvm::SmallString<256> QName; + for (const auto &Entry : IncludeFiles) + if (const Symbol *S = Symbols.find(Entry.first)) { + QName = S->Scope; + QName.append(S->Name); + if (auto Header = getIncludeHeader(QName, Entry.second)) { + Symbol NewSym = *S; + NewSym.IncludeHeaders.push_back({*Header, 1}); + Symbols.insert(NewSym); + } + } + const auto &SM = ASTCtx->getSourceManager(); llvm::DenseMap<FileID, std::string> URICache; auto GetURI = [&](FileID FID) -> llvm::Optional<std::string> { @@ -464,7 +481,7 @@ } return Found->second; }; - + // Populate Refs slab from DeclRefs. if (auto MainFileURI = GetURI(SM.getMainFileID())) { for (const auto &It : DeclRefs) { if (auto ID = getSymbolID(It.first)) { @@ -492,6 +509,7 @@ DeclRefs.clear(); FilesToIndexCache.clear(); HeaderIsSelfContainedCache.clear(); + IncludeFiles.clear(); } const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID, @@ -556,16 +574,10 @@ std::string ReturnType = getReturnType(*CCS); S.ReturnType = ReturnType; - std::string Include; - if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) { + if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) // Use the expansion location to get the #include header since this is // where the symbol is exposed. - if (auto Header = getIncludeHeader( - QName, SM.getDecomposedExpansionLoc(ND.getLocation()).first)) - Include = std::move(*Header); - } - if (!Include.empty()) - S.IncludeHeaders.emplace_back(Include, 1); + IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(ND.getLocation()).first; llvm::Optional<OpaqueType> TypeStorage; if (S.Flags & Symbol::IndexedForCodeCompletion) { Index: clangd/index/Symbol.h =================================================================== --- clangd/index/Symbol.h +++ clangd/index/Symbol.h @@ -204,10 +204,13 @@ /// This is a deep copy: underlying strings will be owned by the slab. void insert(const Symbol &S); - /// Returns the symbol with an ID, if it exists. Valid until next insert(). + /// Removes the symbol with an ID, if it exists. + void erase(const SymbolID &ID) { Symbols.erase(ID); } + + /// Returns the symbol with an ID, if it exists. Valid until insert/remove. const Symbol *find(const SymbolID &ID) { - auto I = SymbolIndex.find(ID); - return I == SymbolIndex.end() ? nullptr : &Symbols[I->second]; + auto I = Symbols.find(ID); + return I == Symbols.end() ? nullptr : &I->second; } /// Consumes the builder to finalize the slab. @@ -217,9 +220,8 @@ llvm::BumpPtrAllocator Arena; /// Intern table for strings. Contents are on the arena. llvm::UniqueStringSaver UniqueStrings; - std::vector<Symbol> Symbols; /// Values are indices into Symbols vector. - llvm::DenseMap<SymbolID, size_t> SymbolIndex; + llvm::DenseMap<SymbolID, Symbol> Symbols; }; private: Index: clangd/index/Symbol.cpp =================================================================== --- clangd/index/Symbol.cpp +++ clangd/index/Symbol.cpp @@ -48,27 +48,23 @@ } void SymbolSlab::Builder::insert(const Symbol &S) { - auto R = SymbolIndex.try_emplace(S.ID, Symbols.size()); - if (R.second) { - Symbols.push_back(S); - own(Symbols.back(), UniqueStrings); - } else { - auto &Copy = Symbols[R.first->second] = S; - own(Copy, UniqueStrings); - } + own(Symbols[S.ID] = S, UniqueStrings); } SymbolSlab SymbolSlab::Builder::build() && { - Symbols = {Symbols.begin(), Symbols.end()}; // Force shrink-to-fit. - // Sort symbols so the slab can binary search over them. - llvm::sort(Symbols, + // Sort symbols into vector so the slab can binary search over them. + std::vector<Symbol> SortedSymbols; + SortedSymbols.reserve(Symbols.size()); + for (auto &Entry : Symbols) + SortedSymbols.push_back(std::move(Entry.second)); + llvm::sort(SortedSymbols, [](const Symbol &L, const Symbol &R) { return L.ID < R.ID; }); // We may have unused strings from overwritten symbols. Build a new arena. llvm::BumpPtrAllocator NewArena; llvm::UniqueStringSaver Strings(NewArena); - for (auto &S : Symbols) + for (auto &S : SortedSymbols) own(S, Strings); - return SymbolSlab(std::move(NewArena), std::move(Symbols)); + return SymbolSlab(std::move(NewArena), std::move(SortedSymbols)); } } // namespace clangd
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits