This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sammccall marked 3 inline comments as done.
Closed by commit rG91679c95bbed: [clangd] Include macro expansions in
documentSymbol hierarchy (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D97615?vs=326900&id=327472#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97615/new/
https://reviews.llvm.org/D97615
Files:
clang-tools-extra/clangd/FindSymbols.cpp
clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -662,7 +662,76 @@
WithDetail("(unnamed)"))))))));
}
-TEST(DocumentSymbols, FromMacro) {
+TEST(DocumentSymbols, Macro) {
+ struct Test {
+ const char *Code;
+ testing::Matcher<DocumentSymbol> Matcher;
+ } Tests[] = {
+ {
+ R"cpp(
+ // Basic macro that generates symbols.
+ #define DEFINE_FLAG(X) bool FLAGS_##X; bool FLAGS_no##X
+ DEFINE_FLAG(pretty);
+ )cpp",
+ AllOf(WithName("DEFINE_FLAG"), WithDetail("(pretty)"),
+ Children(WithName("FLAGS_pretty"), WithName("FLAGS_nopretty"))),
+ },
+ {
+ R"cpp(
+ // Hierarchy is determined by primary (name) location.
+ #define ID(X) X
+ namespace ID(ns) { int ID(y); }
+ )cpp",
+ AllOf(WithName("ID"), WithDetail("(ns)"),
+ Children(AllOf(WithName("ns"),
+ Children(AllOf(WithName("ID"), WithDetail("(y)"),
+ Children(WithName("y"))))))),
+ },
+ {
+ R"cpp(
+ // More typical example where macro only generates part of a decl.
+ #define TEST(A, B) class A##_##B { void go(); }; void A##_##B::go()
+ TEST(DocumentSymbols, Macro) { }
+ )cpp",
+ AllOf(WithName("TEST"), WithDetail("(DocumentSymbols, Macro)"),
+ Children(AllOf(WithName("DocumentSymbols_Macro"),
+ Children(WithName("go"))),
+ WithName("DocumentSymbols_Macro::go"))),
+ },
+ {
+ R"cpp(
+ // Nested macros.
+ #define NAMESPACE(NS, BODY) namespace NS { BODY }
+ NAMESPACE(a, NAMESPACE(b, int x;))
+ )cpp",
+ AllOf(
+ WithName("NAMESPACE"), WithDetail("(a, NAMESPACE(b, int x;))"),
+ Children(AllOf(
+ WithName("a"),
+ Children(AllOf(WithName("NAMESPACE"),
+ // FIXME: nested expansions not in TokenBuffer
+ WithDetail(""),
+ Children(AllOf(WithName("b"),
+ Children(WithName("x"))))))))),
+ },
+ {
+ R"cpp(
+ // Macro invoked from body is not exposed.
+ #define INNER(X) int X
+ #define OUTER(X) INNER(X)
+ OUTER(foo);
+ )cpp",
+ AllOf(WithName("OUTER"), WithDetail("(foo)"),
+ Children(WithName("foo"))),
+ },
+ };
+ for (const Test &T : Tests) {
+ auto TU = TestTU::withCode(T.Code);
+ EXPECT_THAT(getSymbols(TU.build()), ElementsAre(T.Matcher)) << T.Code;
+ }
+}
+
+TEST(DocumentSymbols, RangeFromMacro) {
TestTU TU;
Annotations Main(R"(
#define FF(name) \
@@ -671,9 +740,9 @@
$expansion1[[FF]](abc);
#define FF2() \
- class Test {};
+ class Test {}
- $expansion2[[FF2]]();
+ $expansion2parens[[$expansion2[[FF2]]()]];
#define FF3() \
void waldo()
@@ -683,13 +752,21 @@
}]]
)");
TU.Code = Main.code().str();
- EXPECT_THAT(getSymbols(TU.build()),
- ElementsAre(AllOf(WithName("abc_Test"), WithDetail("class"),
- SymNameRange(Main.range("expansion1"))),
- AllOf(WithName("Test"), WithDetail("class"),
- SymNameRange(Main.range("expansion2"))),
- AllOf(WithName("waldo"), WithDetail("void ()"),
- SymRange(Main.range("fullDef")))));
+ EXPECT_THAT(
+ getSymbols(TU.build()),
+ ElementsAre(
+ AllOf(WithName("FF"), WithDetail("(abc)"),
+ Children(AllOf(WithName("abc_Test"), WithDetail("class"),
+ SymNameRange(Main.range("expansion1"))))),
+ AllOf(WithName("FF2"), WithDetail("()"),
+ SymNameRange(Main.range("expansion2")),
+ SymRange(Main.range("expansion2parens")),
+ Children(AllOf(WithName("Test"), WithDetail("class"),
+ SymNameRange(Main.range("expansion2"))))),
+ AllOf(WithName("FF3"), WithDetail("()"),
+ SymRange(Main.range("fullDef")),
+ Children(AllOf(WithName("waldo"), WithDetail("void ()"),
+ SymRange(Main.range("fullDef")))))));
}
TEST(DocumentSymbols, FuncTemplates) {
Index: clang-tools-extra/clangd/FindSymbols.cpp
===================================================================
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -289,21 +289,103 @@
/// - visiting decls is actually simple, so we don't hit the complicated
/// cases that RAV mostly helps with (types, expressions, etc.)
class DocumentOutline {
+ // A DocumentSymbol we're constructing.
+ // We use this instead of DocumentSymbol directly so that we can keep track
+ // of the nodes we insert for macros.
+ class SymBuilder {
+ std::vector<SymBuilder> Children;
+ DocumentSymbol Symbol; // Symbol.children is empty, use Children instead.
+ // Macro expansions that this node or its parents are associated with.
+ // (Thus we will never create further children for these expansions).
+ llvm::SmallVector<SourceLocation> EnclosingMacroLoc;
+
+ public:
+ DocumentSymbol build() && {
+ for (SymBuilder &C : Children) {
+ Symbol.children.push_back(std::move(C).build());
+ // Expand range to ensure children nest properly, which editors expect.
+ // This can fix some edge-cases in the AST, but is vital for macros.
+ // A macro expansion "contains" AST node if it covers the node's primary
+ // location, but it may not span the node's whole range.
+ Symbol.range.start =
+ std::min(Symbol.range.start, Symbol.children.back().range.start);
+ Symbol.range.end =
+ std::max(Symbol.range.end, Symbol.children.back().range.end);
+ }
+ return std::move(Symbol);
+ }
+
+ // Add a symbol as a child of the current one.
+ SymBuilder &addChild(DocumentSymbol S) {
+ Children.emplace_back();
+ Children.back().EnclosingMacroLoc = EnclosingMacroLoc;
+ Children.back().Symbol = std::move(S);
+ return Children.back();
+ }
+
+ // Get an appropriate container for children of this symbol that were
+ // expanded from a macro (whose spelled name is Tok).
+ //
+ // This may return:
+ // - a macro symbol child of this (either new or previously created)
+ // - this scope itself, if it *is* the macro symbol or is nested within it
+ SymBuilder &inMacro(const syntax::Token &Tok, const SourceManager &SM,
+ llvm::Optional<syntax::TokenBuffer::Expansion> Exp) {
+ if (llvm::is_contained(EnclosingMacroLoc, Tok.location()))
+ return *this;
+ // If there's an existing child for this macro, we expect it to be last.
+ if (!Children.empty() && !Children.back().EnclosingMacroLoc.empty() &&
+ Children.back().EnclosingMacroLoc.back() == Tok.location())
+ return Children.back();
+
+ DocumentSymbol Sym;
+ Sym.name = Tok.text(SM).str();
+ Sym.kind = SymbolKind::Null; // There's no suitable kind!
+ Sym.range = Sym.selectionRange =
+ halfOpenToRange(SM, Tok.range(SM).toCharRange(SM));
+
+ // FIXME: Exp is currently unavailable for nested expansions.
+ if (Exp) {
+ // Full range covers the macro args.
+ Sym.range = halfOpenToRange(SM, CharSourceRange::getCharRange(
+ Exp->Spelled.front().location(),
+ Exp->Spelled.back().endLocation()));
+ // Show macro args as detail.
+ llvm::raw_string_ostream OS(Sym.detail);
+ const syntax::Token *Prev = nullptr;
+ for (const auto &Tok : Exp->Spelled.drop_front()) {
+ // Don't dump arbitrarily long macro args.
+ if (OS.tell() > 80) {
+ OS << " ...)";
+ break;
+ }
+ if (Prev && Prev->endLocation() != Tok.location())
+ OS << ' ';
+ OS << Tok.text(SM);
+ Prev = &Tok;
+ }
+ }
+ SymBuilder &Child = addChild(std::move(Sym));
+ Child.EnclosingMacroLoc.push_back(Tok.location());
+ return Child;
+ }
+ };
+
public:
DocumentOutline(ParsedAST &AST) : AST(AST) {}
/// Builds the document outline for the generated AST.
std::vector<DocumentSymbol> build() {
- std::vector<DocumentSymbol> Results;
+ SymBuilder DummyRoot;
for (auto &TopLevel : AST.getLocalTopLevelDecls())
- traverseDecl(TopLevel, Results);
- return Results;
+ traverseDecl(TopLevel, DummyRoot);
+ return std::move(std::move(DummyRoot).build().children);
}
private:
enum class VisitKind { No, OnlyDecl, OnlyChildren, DeclAndChildren };
- void traverseDecl(Decl *D, std::vector<DocumentSymbol> &Results) {
+ void traverseDecl(Decl *D, SymBuilder &Parent) {
// Skip symbols which do not originate from the main file.
if (!isInsideMainFile(D->getLocation(), AST.getSourceManager()))
return;
@@ -319,27 +401,76 @@
return;
if (Visit == VisitKind::OnlyChildren)
- return traverseChildren(D, Results);
+ return traverseChildren(D, Parent);
auto *ND = llvm::cast<NamedDecl>(D);
auto Sym = declToSym(AST.getASTContext(), *ND);
if (!Sym)
return;
- Results.push_back(std::move(*Sym));
+ SymBuilder &MacroParent = possibleMacroContainer(D->getLocation(), Parent);
+ SymBuilder &Child = MacroParent.addChild(std::move(*Sym));
if (Visit == VisitKind::OnlyDecl)
return;
assert(Visit == VisitKind::DeclAndChildren && "Unexpected VisitKind");
- traverseChildren(ND, Results.back().children);
+ traverseChildren(ND, Child);
+ }
+
+ // Determines where a decl should appear in the DocumentSymbol hierarchy.
+ //
+ // This is usually a direct child of the relevant AST parent.
+ // But we may also insert nodes for macros. Given:
+ // #define DECLARE_INT(V) int v;
+ // namespace a { DECLARE_INT(x) }
+ // We produce:
+ // Namespace a
+ // Macro DECLARE_INT(x)
+ // Variable x
+ //
+ // In the absence of macros, this method simply returns Parent.
+ // Otherwise it may return a macro expansion node instead.
+ // Each macro only has at most one node in the hierarchy, even if it expands
+ // to multiple decls.
+ SymBuilder &possibleMacroContainer(SourceLocation TargetLoc,
+ SymBuilder &Parent) {
+ const auto &SM = AST.getSourceManager();
+ // Look at the path of macro-callers from the token to the main file.
+ // Note that along these paths we see the "outer" macro calls first.
+ SymBuilder *CurParent = &Parent;
+ for (SourceLocation Loc = TargetLoc; Loc.isMacroID();
+ Loc = SM.getImmediateMacroCallerLoc(Loc)) {
+ // Find the virtual macro body that our token is being substituted into.
+ FileID MacroBody;
+ if (SM.isMacroArgExpansion(Loc)) {
+ // Loc is part of a macro arg being substituted into a macro body.
+ MacroBody = SM.getFileID(SM.getImmediateExpansionRange(Loc).getBegin());
+ } else {
+ // Loc is already in the macro body.
+ MacroBody = SM.getFileID(Loc);
+ }
+ // The macro body is being substituted for a macro expansion, whose
+ // first token is the name of the macro.
+ SourceLocation MacroName =
+ SM.getSLocEntry(MacroBody).getExpansion().getExpansionLocStart();
+ // Only include the macro expansion in the outline if it was written
+ // directly in the main file, rather than expanded from another macro.
+ if (!MacroName.isValid() || !MacroName.isFileID())
+ continue;
+ // All conditions satisfied, add the macro.
+ if (auto *Tok = AST.getTokens().spelledTokenAt(MacroName))
+ CurParent = &CurParent->inMacro(
+ *Tok, SM, AST.getTokens().expansionStartingAt(Tok));
+ }
+ return *CurParent;
}
- void traverseChildren(Decl *D, std::vector<DocumentSymbol> &Results) {
+ void traverseChildren(Decl *D, SymBuilder &Builder) {
auto *Scope = llvm::dyn_cast<DeclContext>(D);
if (!Scope)
return;
for (auto *C : Scope->decls())
- traverseDecl(C, Results);
+ traverseDecl(C, Builder);
}
VisitKind shouldVisit(Decl *D) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
