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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
