This revision was automatically updated to reflect the committed changes. Closed by commit rL325764: [clangd] Not collect include headers for dynamic index for now. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits.
Repository: rL LLVM https://reviews.llvm.org/D43550 Files: clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/clangd/index/FileIndex.cpp clang-tools-extra/trunk/clangd/index/Index.h clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp @@ -585,7 +585,7 @@ runSymbolCollector("class Foo {};", /*Main=*/""); EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI), - IncludeHeader("")))); + IncludeHeader(TestHeaderURI)))); } #ifndef LLVM_ON_WIN32 Index: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp @@ -181,19 +181,19 @@ EXPECT_THAT(match(M, Req), UnorderedElementsAre("X")); } -#ifndef LLVM_ON_WIN32 -TEST(FileIndexTest, CanonicalizeSystemHeader) { +TEST(FileIndexTest, NoIncludeCollected) { FileIndex M; - std::string File = testPath("bits/basic_string"); - M.update(File, build(File, "class string {};").getPointer()); + M.update("f", build("f", "class string {};").getPointer()); FuzzyFindRequest Req; Req.Query = ""; + bool SeenSymbol = false; M.fuzzyFind(Req, [&](const Symbol &Sym) { - EXPECT_EQ(Sym.Detail->IncludeHeader, "<string>"); + EXPECT_TRUE(Sym.Detail->IncludeHeader.empty()); + SeenSymbol = true; }); + EXPECT_TRUE(SeenSymbol); } -#endif } // namespace } // namespace clangd Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp =================================================================== --- clang-tools-extra/trunk/clangd/CodeComplete.cpp +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp @@ -286,24 +286,18 @@ I.documentation = D->Documentation; if (I.detail.empty()) I.detail = D->CompletionDetail; - // We only insert #include for items with details, since we can't tell - // whether the file URI of the canonical declaration would be the - // canonical #include without checking IncludeHeader in the detail. // FIXME: delay creating include insertion command to // "completionItem/resolve", when it is supported - if (!D->IncludeHeader.empty() || - !IndexResult->CanonicalDeclaration.FileURI.empty()) { + if (!D->IncludeHeader.empty()) { // LSP favors additionalTextEdits over command. But we are still using // command here because it would be expensive to calculate #include // insertion edits for all candidates, and the include insertion edit // is unlikely to conflict with the code completion edits. Command Cmd; // Command title is not added since this is not a user-facing command. Cmd.command = ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE; IncludeInsertion Insertion; - Insertion.header = D->IncludeHeader.empty() - ? IndexResult->CanonicalDeclaration.FileURI - : D->IncludeHeader; + Insertion.header = D->IncludeHeader; Insertion.textDocument.uri = URIForFile(FileName); Cmd.includeInsertion = std::move(Insertion); I.command = std::move(Cmd); Index: clang-tools-extra/trunk/clangd/index/FileIndex.cpp =================================================================== --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp @@ -15,29 +15,22 @@ namespace clangd { namespace { -const CanonicalIncludes *canonicalIncludesForSystemHeaders() { - static const auto *Includes = [] { - auto *I = new CanonicalIncludes(); - addSystemHeadersMapping(I); - return I; - }(); - return Includes; -} - /// Retrieves namespace and class level symbols in \p Decls. std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx, std::shared_ptr<Preprocessor> PP, llvm::ArrayRef<const Decl *> Decls) { SymbolCollector::Options CollectorOpts; // Although we do not index symbols in main files (e.g. cpp file), information // in main files like definition locations of class declarations will still be // collected; thus, the index works for go-to-definition. - // FIXME(ioeric): handle IWYU pragma for dynamic index. We might want to make - // SymbolCollector always provides include canonicalization (e.g. IWYU, STL). // FIXME(ioeric): get rid of `IndexMainFiles` as this is always set to false. CollectorOpts.IndexMainFiles = false; - CollectorOpts.CollectIncludePath = true; - CollectorOpts.Includes = canonicalIncludesForSystemHeaders(); + // FIXME(ioeric): we might also want to collect include headers. We would need + // to make sure all includes are canonicalized (with CanonicalIncludes), which + // is not trivial given the current way of collecting symbols: we only have + // AST at this point, but we also need preprocessor callbacks (e.g. + // CommentHandler for IWYU pragma) to canonicalize includes. + CollectorOpts.CollectIncludePath = false; auto Collector = std::make_shared<SymbolCollector>(std::move(CollectorOpts)); Collector->setPreprocessor(std::move(PP)); Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp =================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp +++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp @@ -150,8 +150,9 @@ } } -/// Gets a canonical include (<header> or "header") for header of \p Loc. -/// Returns None if the header has no canonical include. +/// Gets a canonical include (URI of the header or <header> or "header") for +/// header of \p Loc. +/// Returns None if fails to get include header for \p Loc. /// FIXME: we should handle .inc files whose symbols are expected be exported by /// their containing headers. llvm::Optional<std::string> @@ -167,10 +168,8 @@ ? Mapped.str() : ("\"" + Mapped + "\"").str(); } - // If the header path is the same as the file path of the declaration, we skip - // storing the #include path; users can use the URI in declaration location to - // calculate the #include path. - return llvm::None; + + return toURI(SM, SM.getFilename(Loc), Opts); } // Return the symbol location of the given declaration `D`. Index: clang-tools-extra/trunk/clangd/index/Index.h =================================================================== --- clang-tools-extra/trunk/clangd/index/Index.h +++ clang-tools-extra/trunk/clangd/index/Index.h @@ -162,8 +162,8 @@ /// directly. When this is a URI, the exact #include path needs to be /// calculated according to the URI scheme. /// - /// If empty, FileURI in CanonicalDeclaration should be used to calculate - /// the #include path. + /// This is a canonical include for the symbol and can be different from + /// FileURI in the CanonicalDeclaration. llvm::StringRef IncludeHeader; };
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits