[PATCH] D97615: [clangd] Include macro expansions in documentSymbol hierarchy

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, LGTM. with a couple nits and asking for some extra clarifications :)



================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:283
+  public:
+    SymBuilder() = default;
+
----------------
nit: drop this one?


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:290
+        // 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).
----------------
this comment feels a little out-of-place, maybe move it next to `possibleMacroContainer` call?


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:327
+      Sym.selectionRange = halfOpenToRange(SM, Tok.range(SM).toCharRange(SM));
+      if (Exp)
+        Sym.range = halfOpenToRange(SM, CharSourceRange::getCharRange(
----------------
nit:
```
Sym.range = Sym.selectionRange = halfOpenToRange(SM, Tok.range(SM).toCharRange(SM));
if (Exp) {
  Sym.range = .... // cover Exp;
  // Populate Sym.detail.
}
```


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:402
+
+  // Determines where a decl should appear in the DocumentSymbol hierarchy.
+  //
----------------
also mention that `there's at most one node for each macro invocation in the document symbol hierarchy, even if it yields multiple decls`


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:424
+         Loc = SM.getImmediateMacroCallerLoc(Loc)) {
+      FileID MacroBody;
+      if (SM.isMacroArgExpansion(Loc)) {
----------------
nit:
```
SourceLocation ExpansionLoc = Loc;
// If ExpansionLoc is inside a macro argument, use the outer macro invocation instead. e.g:
//    #define ID(X) X
//    ID(int y); // chose location of ID, not X
if(SM.isMacroArgExpansion(ExpansionLoc))
  ExpansionLoc = SM.getImmediateExpansionRange(Loc).getBegin();
FileID MacroBody = SM.getFileID(ExpansionLoc);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97615/new/

https://reviews.llvm.org/D97615