sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
Browsing macro-generated symbols is confusing.
On the one hand, it seems very *useful* to be able to see the summary of
symbols that were generated.
On the other hand, some macros spew a lot of confusing symbols into the
namespace and when used repeatedly (ABSL_FLAG) can create a lot of spam
that's hard to navigate.
Design constraints:
- the macro expansion tree need not align with the AST, though it often does in
practice. We address this by defining the nesting based on the *primary*
location of decls, rather than their ranges.
- DocumentSymbol.children[*].range should nest within DocumentSymbol.range
(This constraint is not in LSP "breadcrumbs" breaks without it) We adjust macro
ranges so they cover their "children", rather than just the macro expansion
- LSP does not have a "macro expansion" symbolkind, nor does it allow a symbol
to have no kind. I've arbitrarily picked "null" as this is unlikely to conflict
with anything useful.
This patch makes all macros and children visible for simplicity+consistency,
though in some cases it may be better to elide the macro node.
We may consider adding heuristics for this in future (e.g. when it expands
to one decl only?) but it doesn't seem clear-cut to me.
Repository:
rG LLVM Github Monorepo
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
@@ -269,21 +269,105 @@
/// - 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:
+ SymBuilder() = default;
+
+ 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.
+ // (We determine macro hierarchy using primary location only, and in
+ // general macros need not nest with AST nodes).
+ 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.selectionRange = halfOpenToRange(SM, Tok.range(SM).toCharRange(SM));
+ if (Exp)
+ Sym.range = halfOpenToRange(SM, CharSourceRange::getCharRange(
+ Exp->Spelled.front().location(),
+ Exp->Spelled.back().endLocation()));
+ else
+ Sym.range = Sym.selectionRange;
+ // FIXME: Exp is currently unavailable for nested expansions.
+ if (Exp) {
+ // 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;
@@ -299,27 +383,73 @@
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 will return the child list of a DocumentSymbol representing
+ // a macro expansion, which is itself a child of Parent.
+ 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)) {
+ FileID MacroBody;
+ if (SM.isMacroArgExpansion(Loc)) {
+ // Loc is an arg being expanded by a macro body, which is in turn being
+ // expanded by a macro call.
+ MacroBody = SM.getFileID(SM.getImmediateExpansionRange(Loc).getBegin());
+ } else {
+ // Loc is itself in the macro body, which is expanded by the macro call.
+ MacroBody = SM.getFileID(Loc);
+ }
+ 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
