ilya-biryukov added inline comments.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:104
// *definitions* in the preamble region of the main file).
class CollectMainFileMacroExpansions : public PPCallbacks {
const SourceManager &SM;
----------------
hokein wrote:
> The name doesn't fit into what it does anymore, maybe rename to
> `CollectMainFileMacroLocations`?
Agree, maybe even shorter - `CollectMainFileMacros`?
================
Comment at: clang-tools-extra/clangd/ParsedAST.h:125
+ /// preamble.
/// Does not include expansions from inside other macro expansions.
std::vector<SourceLocation> MainFileMacroExpLocs;
----------------
NIT: s/Does not include expansions/Does not include **locations**?
To avoid confusion, since they are **definitions and expansions** now.
================
Comment at: clang-tools-extra/clangd/ParsedAST.h:126
/// Does not include expansions from inside other macro expansions.
std::vector<SourceLocation> MainFileMacroExpLocs;
// Data, stored after parsing.
----------------
Could we you rename this to `MacroIdentifierLocs`? Or something similar not
mentioning expansions in the name.
`MacroExpLocs` could still be interpreted as locations of expansions, rather
than locations of all macro identifiers.
================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:234
+ namespace {
+ #define ^MACRO_ARGS(X, Y) X Y
^ID(int A);
----------------
We should probably keep the test as is and make sure we also collect inside the
preamble.
It's ok to leave a FIXME and do this in a separate change, but let's not change
the test to hide the fact that we are not highlighting macros inside the
preamble anymore.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67264/new/
https://reviews.llvm.org/D67264
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
