ioeric updated this revision to Diff 162897.
ioeric marked 3 inline comments as done.
ioeric retitled this revision from "[clangd] Support multiple #include headers
in one symbol." to "[clangd] *Prototype* Support multiple #include headers in
one symbol.".
ioeric edited the summary of this revision.
ioeric added a comment.
Herald added a subscriber: mgrang.
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51291
Files:
clangd/CodeComplete.cpp
clangd/index/Index.cpp
clangd/index/Index.h
clangd/index/Merge.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===================================================================
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -53,7 +53,11 @@
MATCHER_P(DeclURI, P, "") { return arg.CanonicalDeclaration.FileURI == P; }
MATCHER_P(DefURI, P, "") { return arg.Definition.FileURI == P; }
MATCHER_P(IncludeHeader, P, "") {
- return arg.Detail && arg.Detail->IncludeHeader == P;
+ return (arg.IncludeHeaders.size() == 1) &&
+ (arg.IncludeHeaders.begin()->IncludeHeader == P);
+}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
}
MATCHER_P(DeclRange, Pos, "") {
return std::tie(arg.CanonicalDeclaration.Start.Line,
@@ -696,6 +700,11 @@
Line: 1
Column: 1
IsIndexedForCodeCompletion: true
+IncludeHeaders:
+ - Header: 'include1'
+ References: 7
+ - Header: 'include2'
+ References: 3
Detail:
Documentation: 'Foo doc'
ReturnType: 'int'
@@ -730,6 +739,11 @@
Doc("Foo doc"), ReturnType("int"),
DeclURI("file:///path/foo.h"),
ForCodeCompletion(true))));
+ auto &Sym1 = *Symbols1.begin();
+ assert(Sym1.Detail);
+ EXPECT_THAT(Sym1.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("include1", 7u),
+ IncludeHeaderWithRef("include2", 3u)));
auto Symbols2 = symbolsFromYAML(YAML2);
EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
QName("clang::Foo2"), Labeled("Foo2-sig"),
@@ -751,9 +765,10 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI),
- IncludeHeader(TestHeaderURI))));
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI(TestHeaderURI))));
+ EXPECT_THAT(Symbols.begin()->IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef(TestHeaderURI, 1u)));
}
#ifndef _WIN32
Index: unittests/clangd/IndexTests.cpp
===================================================================
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -243,6 +243,48 @@
EXPECT_EQ(M.Name, "right");
}
+MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
+ return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
+}
+
+TEST(MergeTest, MergeIncludesOnDifferentDefinitions) {
+ Symbol L, R;
+ L.Name = "left";
+ R.Name = "right";
+ L.ID = R.ID = SymbolID("hello");
+ L.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("common", 1);
+ R.IncludeHeaders.emplace_back("new", 1);
+
+ Symbol::Details Scratch;
+
+ // Both have no definition.
+ Symbol M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Only merge references of the same includes but do not merge new #includes.
+ L.Definition.FileURI = "file:/left.h";
+ M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u)));
+
+ // Definitions are the same.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+
+ // Definitions are different.
+ R.Definition.FileURI = "file:/right.h";
+ M = mergeSymbol(L, R, &Scratch);
+ EXPECT_THAT(M.IncludeHeaders,
+ UnorderedElementsAre(IncludeHeaderWithRef("common", 2u),
+ IncludeHeaderWithRef("new", 1u)));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===================================================================
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -177,7 +177,7 @@
Req.Query = "";
bool SeenSymbol = false;
M.fuzzyFind(Req, [&](const Symbol &Sym) {
- EXPECT_TRUE(Sym.Detail->IncludeHeader.empty());
+ EXPECT_TRUE(Sym.IncludeHeaders.empty());
SeenSymbol = true;
});
EXPECT_TRUE(SeenSymbol);
Index: unittests/clangd/CodeCompleteTests.cpp
===================================================================
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -568,7 +568,8 @@
auto BarURI = URI::createFile(BarHeader).toString();
Symbol Sym = cls("ns::X");
Sym.CanonicalDeclaration.FileURI = BarURI;
- Scratch.IncludeHeader = BarURI;
+ Sym.IncludeHeaders.emplace_back(BarURI, 1);
+
Sym.Detail = &Scratch;
// Shoten include path based on search dirctory and insert.
auto Results = completions(Server,
@@ -602,7 +603,10 @@
auto BarURI = URI::createFile(BarHeader).toString();
SymX.CanonicalDeclaration.FileURI = BarURI;
SymY.CanonicalDeclaration.FileURI = BarURI;
- Scratch.IncludeHeader = "<bar>";
+
+ SymX.IncludeHeaders.emplace_back("<bar>", 1);
+ SymY.IncludeHeaders.emplace_back("<bar>", 1);
+
SymX.Detail = &Scratch;
SymY.Detail = &Scratch;
// Shoten include path based on search dirctory and insert.
@@ -1130,7 +1134,8 @@
Symbol::Details Detail;
std::string DeclFile = URI::createFile(testPath("foo")).toString();
NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
- Detail.IncludeHeader = "<foo>";
+ NoArgsGFunc.IncludeHeaders.emplace_back("<foo>", 1);
+
NoArgsGFunc.Detail = &Detail;
EXPECT_THAT(
completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
@@ -1782,6 +1787,29 @@
ASSERT_EQ(Reqs3.size(), 2u);
}
+TEST(CompletionTest, MultipleIncludeHeadersNoInsertion) {
+ std::string DeclFile = URI::createFile(testPath("foo")).toString();
+ Symbol sym = func("Func");
+ sym.CanonicalDeclaration.FileURI = DeclFile;
+ sym.IncludeHeaders.emplace_back("<foo>", 2);
+ sym.IncludeHeaders.emplace_back("<bar>", 1);
+
+ EXPECT_THAT(completions("Fun^", {sym}).Completions,
+ UnorderedElementsAre(AllOf(Named("Func"), Not(InsertInclude()))));
+}
+
+TEST(CompletionTest, MultipleIncludeHeadersAndInsert) {
+ std::string DeclFile = URI::createFile(testPath("foo")).toString();
+ Symbol sym = func("Func");
+ sym.CanonicalDeclaration.FileURI = DeclFile;
+ sym.IncludeHeaders.emplace_back("<foo>", 2);
+ sym.IncludeHeaders.emplace_back("<bar>", 1000);
+
+ EXPECT_THAT(
+ completions("Fun^", {sym}).Completions,
+ UnorderedElementsAre(AllOf(Named("Func"), InsertInclude("<bar>"))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/SymbolYAML.cpp
===================================================================
--- clangd/index/SymbolYAML.cpp
+++ clangd/index/SymbolYAML.cpp
@@ -10,11 +10,13 @@
#include "SymbolYAML.h"
#include "Index.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/raw_ostream.h"
LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(clang::clangd::Symbol)
+LLVM_YAML_IS_SEQUENCE_VECTOR(clang::clangd::Symbol::IncludeHeaderWithReferences)
namespace llvm {
namespace yaml {
@@ -70,7 +72,15 @@
static void mapping(IO &io, Symbol::Details &Detail) {
io.mapOptional("Documentation", Detail.Documentation);
io.mapOptional("ReturnType", Detail.ReturnType);
- io.mapOptional("IncludeHeader", Detail.IncludeHeader);
+ }
+};
+
+template <>
+struct MappingTraits<clang::clangd::Symbol::IncludeHeaderWithReferences> {
+ static void mapping(IO &io,
+ clang::clangd::Symbol::IncludeHeaderWithReferences &Inc) {
+ io.mapRequired("Header", Inc.IncludeHeader);
+ io.mapRequired("References", Inc.References);
}
};
@@ -112,6 +122,7 @@
false);
IO.mapOptional("Signature", Sym.Signature);
IO.mapOptional("CompletionSnippetSuffix", Sym.CompletionSnippetSuffix);
+ IO.mapOptional("IncludeHeaders", Sym.IncludeHeaders);
IO.mapOptional("Detail", NDetail->Opt);
}
};
Index: clangd/index/SymbolCollector.cpp
===================================================================
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -403,10 +403,12 @@
SM.getExpansionLoc(MI->getDefinitionLoc()), Opts))
Include = std::move(*Header);
}
+ if (!Include.empty())
+ S.IncludeHeaders.emplace_back(Include, 1);
+
S.Signature = Signature;
S.CompletionSnippetSuffix = SnippetSuffix;
Symbol::Details Detail;
- Detail.IncludeHeader = Include;
S.Detail = &Detail;
Symbols.insert(S);
return true;
@@ -489,7 +491,8 @@
Symbol::Details Detail;
Detail.Documentation = Documentation;
Detail.ReturnType = ReturnType;
- Detail.IncludeHeader = Include;
+ if (!Include.empty())
+ S.IncludeHeaders.emplace_back(Include, 1);
S.Detail = &Detail;
S.Origin = Opts.Origin;
Index: clangd/index/Merge.cpp
===================================================================
--- clangd/index/Merge.cpp
+++ clangd/index/Merge.cpp
@@ -100,6 +100,10 @@
// Classes: this is the def itself. Functions: hopefully the header decl.
// If both did (or both didn't), continue to prefer L over R.
bool PreferR = R.Definition && !L.Definition;
+ // Merge include headers only if both have definitions or both have no
+ // definition; otherwise, only accumulate references of common includes.
+ bool MergeIncludes =
+ L.Definition.FileURI.empty() == R.Definition.FileURI.empty();
Symbol S = PreferR ? R : L; // The target symbol we're merging into.
const Symbol &O = PreferR ? L : R; // The "other" less-preferred symbol.
@@ -114,6 +118,18 @@
S.Signature = O.Signature;
if (S.CompletionSnippetSuffix == "")
S.CompletionSnippetSuffix = O.CompletionSnippetSuffix;
+ for (const auto &OI : O.IncludeHeaders) {
+ bool Found = false;
+ for (auto &SI : S.IncludeHeaders) {
+ if (SI.IncludeHeader == OI.IncludeHeader) {
+ Found = true;
+ SI.References += OI.References;
+ break;
+ }
+ }
+ if (!Found && MergeIncludes)
+ S.IncludeHeaders.emplace_back(OI.IncludeHeader, OI.References);
+ }
if (O.Detail) {
if (S.Detail) {
@@ -123,8 +139,6 @@
Scratch->Documentation = O.Detail->Documentation;
if (Scratch->ReturnType == "")
Scratch->ReturnType = O.Detail->ReturnType;
- if (Scratch->IncludeHeader == "")
- Scratch->IncludeHeader = O.Detail->IncludeHeader;
S.Detail = Scratch;
} else
S.Detail = O.Detail;
Index: clangd/index/Index.h
===================================================================
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -16,7 +16,9 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/StringSaver.h"
#include <array>
#include <string>
@@ -200,6 +202,30 @@
/// (When snippets are disabled, the symbol name alone is used).
llvm::StringRef CompletionSnippetSuffix;
+ struct IncludeHeaderWithReferences {
+ IncludeHeaderWithReferences() = default;
+
+ IncludeHeaderWithReferences(llvm::StringRef IncludeHeader,
+ unsigned References)
+ : IncludeHeader(IncludeHeader), References(References) {}
+
+ /// This can be either a URI of the header to be #include'd
+ /// for this symbol, or a literal header quoted with <> or "" that is
+ /// suitable to be included directly. When it is a URI, the exact #include
+ /// path needs to be calculated according to the URI scheme.
+ ///
+ /// Note that the include header is a canonical include for the symbol and
+ /// can be different from FileURI in the CanonicalDeclaration.
+ llvm::StringRef IncludeHeader = "";
+ /// The number of translation units that reference this symbol and include
+ /// this header. This number is only meaningful if aggregated in an index.
+ unsigned References = 0;
+ };
+ /// One Symbol can potentially be incuded via different headers.
+ /// - If we haven't seen a definition, this covers all declarations.
+ /// - If we have seen a definition, this covers declarations visible from
+ /// any definition.
+ llvm::SmallVector<IncludeHeaderWithReferences, 1> IncludeHeaders;
/// Optional symbol details that are not required to be set. For example, an
/// index fuzzy match can return a large number of symbol candidates, and it
/// is preferable to send only core symbol information in the batched results
@@ -211,14 +237,6 @@
/// Type when this symbol is used in an expression. (Short display form).
/// e.g. return type of a function, or type of a variable.
llvm::StringRef ReturnType;
- /// This can be either a URI of the header to be #include'd for this symbol,
- /// or a literal header quoted with <> or "" that is suitable to be included
- /// directly. When this is a URI, the exact #include path needs to be
- /// calculated according to the URI scheme.
- ///
- /// This is a canonical include for the symbol and can be different from
- /// FileURI in the CanonicalDeclaration.
- llvm::StringRef IncludeHeader;
};
// Optional details of the symbol.
Index: clangd/index/Index.cpp
===================================================================
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -9,6 +9,7 @@
#include "Index.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/SHA1.h"
#include "llvm/Support/raw_ostream.h"
@@ -91,14 +92,16 @@
Intern(S.Signature);
Intern(S.CompletionSnippetSuffix);
+ for (auto &I : S.IncludeHeaders)
+ Intern(I.IncludeHeader);
+
if (S.Detail) {
// Copy values of StringRefs into arena.
auto *Detail = Arena.Allocate<Symbol::Details>();
*Detail = *S.Detail;
// Intern the actual strings.
Intern(Detail->Documentation);
Intern(Detail->ReturnType);
- Intern(Detail->IncludeHeader);
// Replace the detail pointer with our copy.
S.Detail = Detail;
}
Index: clangd/CodeComplete.cpp
===================================================================
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -48,6 +48,8 @@
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/ScopedPrinter.h"
+#include <algorithm>
+#include <iterator>
#include <queue>
// We log detailed candidate here if you run with -debug-only=codecomplete.
@@ -285,8 +287,7 @@
}
llvm::Optional<llvm::StringRef> headerToInsertIfNotPresent() const {
- if (!IndexResult || !IndexResult->Detail ||
- IndexResult->Detail->IncludeHeader.empty())
+ if (!IndexResult || IndexResult->IncludeHeaders.empty())
return llvm::None;
if (SemaResult && SemaResult->Declaration) {
// Avoid inserting new #include if the declaration is found in the current
@@ -296,7 +297,26 @@
if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc())))
return llvm::None;
}
- return IndexResult->Detail->IncludeHeader;
+ if (IndexResult->IncludeHeaders.size() == 1)
+ return IndexResult->IncludeHeaders.front().IncludeHeader;
+
+ // Pick the most popular include, only if it has way more references than
+ // the rest of includes; otherwise, we just give up and don't insert
+ // includes.
+ // FIXME(ioeric): we might want to store possible #includes in the
+ // completion result so that clients can decide what to do with them.
+ auto Includes = IndexResult->IncludeHeaders;
+ assert(Includes.size() > 1);
+ // Sort in descending order by reference count.
+ std::sort(Includes.begin(), Includes.end(),
+ [](const Symbol::IncludeHeaderWithReferences &LHS,
+ const Symbol::IncludeHeaderWithReferences &RHS) {
+ return LHS.References > RHS.References;
+ });
+ if (Includes.begin()->References >
+ 10 * std::next(Includes.begin())->References)
+ return Includes.front().IncludeHeader;
+ return llvm::None;
}
using Bundle = llvm::SmallVector<CompletionCandidate, 4>;
@@ -381,8 +401,7 @@
} else
log("Failed to generate include insertion edits for adding header "
"(FileURI='{0}', IncludeHeader='{1}') into {2}",
- C.IndexResult->CanonicalDeclaration.FileURI,
- C.IndexResult->Detail->IncludeHeader, FileName);
+ C.IndexResult->CanonicalDeclaration.FileURI, *Inserted, FileName);
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
