ioeric created this revision. ioeric added reviewers: ilya-biryukov, sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Pros: o Loading macros from preamble for every completion is slow (see profile). o Calculating macro USR is also slow (see profile). o Sema can provide a lot of macro completion results (e.g. when filter is empty, 60k for some large TUs!). Cons: o Slight memory increase in dynamic index (~1%). o Some extra work during preamble build (should be fine as preamble build and indexAST is way slower). [profile place holder] Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52078 Files: clangd/ClangdServer.cpp clangd/index/FileIndex.cpp clangd/index/FileIndex.h unittests/clangd/CodeCompleteTests.cpp unittests/clangd/FileIndexTests.cpp unittests/clangd/IndexTests.cpp unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp =================================================================== --- unittests/clangd/TestTU.cpp +++ unittests/clangd/TestTU.cpp @@ -51,7 +51,7 @@ std::unique_ptr<SymbolIndex> TestTU::index() const { auto AST = build(); auto Content = indexAST(AST.getASTContext(), AST.getPreprocessorPtr(), - AST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, AST.getLocalTopLevelDecls()); return MemIndex::build(std::move(Content.first), std::move(Content.second)); } Index: unittests/clangd/IndexTests.cpp =================================================================== --- unittests/clangd/IndexTests.cpp +++ unittests/clangd/IndexTests.cpp @@ -244,7 +244,7 @@ Test.Filename = "test.cc"; auto AST = Test.build(); Dyn.update(Test.Filename, &AST.getASTContext(), AST.getPreprocessorPtr(), - AST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, AST.getLocalTopLevelDecls()); // Build static index for test.cc. Test.HeaderCode = HeaderCode; @@ -254,7 +254,7 @@ // Add stale refs for test.cc. StaticIndex.update(Test.Filename, &StaticAST.getASTContext(), StaticAST.getPreprocessorPtr(), - StaticAST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls()); // Add refs for test2.cc Annotations Test2Code(R"(class $Foo[[Foo]] {};)"); @@ -265,7 +265,7 @@ StaticAST = Test2.build(); StaticIndex.update(Test2.Filename, &StaticAST.getASTContext(), StaticAST.getPreprocessorPtr(), - StaticAST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, StaticAST.getLocalTopLevelDecls()); RefsRequest Request; Request.IDs = {Foo.ID}; Index: unittests/clangd/FileIndexTests.cpp =================================================================== --- unittests/clangd/FileIndexTests.cpp +++ unittests/clangd/FileIndexTests.cpp @@ -15,6 +15,7 @@ #include "index/FileIndex.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/PCHContainerOperations.h" +#include "clang/Index/IndexSymbol.h" #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/CompilationDatabase.h" #include "gtest/gtest.h" @@ -135,7 +136,8 @@ File.HeaderFilename = (Basename + ".h").str(); File.HeaderCode = Code; auto AST = File.build(); - M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr()); + M.update(File.Filename, &AST.getASTContext(), AST.getPreprocessorPtr(), + /*IndexMacros=*/true); } TEST(FileIndexTest, CustomizedURIScheme) { @@ -333,23 +335,38 @@ Test.Filename = "test.cc"; auto AST = Test.build(); Index.update(Test.Filename, &AST.getASTContext(), AST.getPreprocessorPtr(), - AST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, AST.getLocalTopLevelDecls()); // Add test2.cc TestTU Test2; Test2.HeaderCode = HeaderCode; Test2.Code = MainCode.code(); Test2.Filename = "test2.cc"; AST = Test2.build(); Index.update(Test2.Filename, &AST.getASTContext(), AST.getPreprocessorPtr(), - AST.getLocalTopLevelDecls()); + /*IndexMacros=*/false, AST.getLocalTopLevelDecls()); EXPECT_THAT(getRefs(Index, Foo.ID), RefsAre({AllOf(RefRange(MainCode.range("foo")), FileURI("unittest:///test.cc")), AllOf(RefRange(MainCode.range("foo")), FileURI("unittest:///test2.cc"))})); } +TEST(FileIndexTest, CollectMacros) { + FileIndex M; + update(M, "f", "#define MAC 1; class X {};"); + + FuzzyFindRequest Req; + Req.Query = ""; + bool SeenSymbol = false; + M.fuzzyFind(Req, [&](const Symbol &Sym) { + EXPECT_EQ(Sym.Name, "MAC"); + EXPECT_EQ(Sym.SymInfo.Kind, index::SymbolKind::Macro); + SeenSymbol = true; + }); + EXPECT_TRUE(SeenSymbol); +} + } // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/CodeCompleteTests.cpp =================================================================== --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -223,12 +223,13 @@ void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) { auto Results = completions( R"cpp( - #define MACRO X - int global_var; int global_func(); + // Make sure this is not in preamble. + #define MACRO X + struct GlobalClass {}; struct ClassWithMembers { @@ -276,11 +277,12 @@ void TestGlobalScopeCompletion(clangd::CodeCompleteOptions Opts) { auto Results = completions( R"cpp( - #define MACRO X - int global_var; int global_func(); + // Make sure this is not in preamble. + #define MACRO X + struct GlobalClass {}; struct ClassWithMembers { @@ -430,10 +432,11 @@ TEST(CompletionTest, Kinds) { auto Results = completions( R"cpp( - #define MACRO X int variable; struct Struct {}; int function(); + // make sure MACRO is not included in preamble. + #define MACRO 10 int X = ^ )cpp", {func("indexFunction"), var("indexVariable"), cls("indexClass")}); @@ -1905,6 +1908,21 @@ UnorderedElementsAre(Named("Clangd_Macro_Test"))); } +TEST(CompletionTest, NoMacroFromPreambleIfIndexIsSet) { + auto Results = completions( + R"cpp( + #define CLANGD_PREAMBLE x + int x = 0; + #define CLANGD_MAIN x + void f() { CLANGD_^ } + )cpp", + {func("CLANGD_INDEX")}); + // Index is overriden in code completion options, so the preamble symbol is + // not seen. + EXPECT_THAT(Results.Completions, UnorderedElementsAre(Named("CLANGD_MAIN"), + Named("CLANGD_INDEX"))); +} + TEST(CompletionTest, DeprecatedResults) { std::string Body = R"cpp( void TestClangd(); Index: clangd/index/FileIndex.h =================================================================== --- clangd/index/FileIndex.h +++ clangd/index/FileIndex.h @@ -67,10 +67,12 @@ /// nullptr, this removes all symbols in the file. /// If \p AST is not null, \p PP cannot be null and it should be the /// preprocessor that was used to build \p AST. + /// If \p IndexMacros is true, also collect macro definitions in \p PP. /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all /// top level decls obtained from \p AST are indexed. void update(PathRef Path, ASTContext *AST, std::shared_ptr<Preprocessor> PP, + bool IndexMacros = false, llvm::Optional<llvm::ArrayRef<Decl *>> TopLevelDecls = llvm::None); private: @@ -86,8 +88,10 @@ /// If URISchemes is empty, the default schemes in SymbolCollector will be used. /// If \p TopLevelDecls is set, only these decls are indexed. Otherwise, all top /// level decls obtained from \p AST are indexed. +/// If \p IndexMacros is true, also collect macro definitions in \p PP. std::pair<SymbolSlab, RefSlab> indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP, + bool IndexMacros = false, llvm::Optional<llvm::ArrayRef<Decl *>> TopLevelDecls = llvm::None, llvm::ArrayRef<std::string> URISchemes = {}); Index: clangd/index/FileIndex.cpp =================================================================== --- clangd/index/FileIndex.cpp +++ clangd/index/FileIndex.cpp @@ -10,14 +10,17 @@ #include "FileIndex.h" #include "Logger.h" #include "SymbolCollector.h" +#include "index/Index.h" +#include "clang/Index/IndexSymbol.h" #include "clang/Index/IndexingAction.h" +#include "clang/Lex/MacroInfo.h" #include "clang/Lex/Preprocessor.h" namespace clang { namespace clangd { std::pair<SymbolSlab, RefSlab> -indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP, +indexAST(ASTContext &AST, std::shared_ptr<Preprocessor> PP, bool IndexMacros, llvm::Optional<llvm::ArrayRef<Decl *>> TopLevelDecls, llvm::ArrayRef<std::string> URISchemes) { SymbolCollector::Options CollectorOpts; @@ -28,6 +31,7 @@ // CommentHandler for IWYU pragma) to canonicalize includes. CollectorOpts.CollectIncludePath = false; CollectorOpts.CountReferences = false; + CollectorOpts.CollectMacro = true; if (!URISchemes.empty()) CollectorOpts.URISchemes = URISchemes; CollectorOpts.Origin = SymbolOrigin::Dynamic; @@ -54,13 +58,26 @@ SymbolCollector Collector(std::move(CollectorOpts)); Collector.setPreprocessor(PP); + Collector.initialize(AST); + + if (IndexMacros) { + // FIXME: also collect macro references? + for (const auto &M : PP->macros()) + if (MacroDirective *MD = M.second.getLatest()) + Collector.handleMacroOccurence( + M.first, MD->getMacroInfo(), + static_cast<unsigned>(index::SymbolRole::Definition), + MD->getLocation()); + } + + // Collector will be re-initialized. index::indexTopLevelDecls(AST, DeclsToIndex, Collector, IndexOpts); const auto &SM = AST.getSourceManager(); const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID()); std::string FileName = MainFileEntry ? MainFileEntry->getName() : ""; - auto Syms = Collector.takeSymbols(); + auto Syms = Collector.takeSymbols();; auto Refs = Collector.takeRefs(); vlog("index {0}AST for {1}: \n" " symbol slab: {2} symbols, {3} bytes\n" @@ -142,13 +159,13 @@ } void FileIndex::update(PathRef Path, ASTContext *AST, - std::shared_ptr<Preprocessor> PP, + std::shared_ptr<Preprocessor> PP, bool IndexMacros, llvm::Optional<llvm::ArrayRef<Decl *>> TopLevelDecls) { if (!AST) { FSymbols.update(Path, nullptr, nullptr); } else { assert(PP); - auto Contents = indexAST(*AST, PP, TopLevelDecls, URISchemes); + auto Contents = indexAST(*AST, PP, IndexMacros, TopLevelDecls, URISchemes); FSymbols.update(Path, llvm::make_unique<SymbolSlab>(std::move(Contents.first)), llvm::make_unique<RefSlab>(std::move(Contents.second))); Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -90,13 +90,14 @@ void onPreambleAST(PathRef Path, ASTContext &Ctx, std::shared_ptr<clang::Preprocessor> PP) override { - This->PreambleIdx.update(Path, &Ctx, std::move(PP)); + This->PreambleIdx.update(Path, &Ctx, std::move(PP), + /*IndexMacros=*/true); } void onMainAST(PathRef Path, ParsedAST &AST) override { - This->MainFileIdx.update(Path, &AST.getASTContext(), - AST.getPreprocessorPtr(), - AST.getLocalTopLevelDecls()); + This->MainFileIdx.update( + Path, &AST.getASTContext(), AST.getPreprocessorPtr(), + /*IndexMacros=*/false, AST.getLocalTopLevelDecls()); } }; return llvm::make_unique<CB>(this);
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits