sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:140
+// locations for the found macros.
+void findReferencedMacros(ParsedAST &AST, ReferencedLocations &Result) {
+ auto Tokens =
----------------
can you add a trace for this and verify that it's not a signficant amount of
time on a macro-heavy file (e.g. unittests?)
locateMacroAt wasn't designed to be called in a loop like this, but it seems
plausible it'll be fine
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:140
+// locations for the found macros.
+void findReferencedMacros(ParsedAST &AST, ReferencedLocations &Result) {
+ auto Tokens =
----------------
sammccall wrote:
> can you add a trace for this and verify that it's not a signficant amount of
> time on a macro-heavy file (e.g. unittests?)
>
> locateMacroAt wasn't designed to be called in a loop like this, but it seems
> plausible it'll be fine
>
iterating over all the tokens and examining them individually seems like we're
doing too much work.
ParsedAST.MainFileMacros.Names already knows the names of all macros referenced
from the main file.
PP.getLocalMacroDirectiveHistory() will give you the last seen definition for a
given macro name. If we want, we can also get the previous definitions.
This seems like it should be close enough, and wade through many fewer tokens?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:144
+ for (const syntax::Token &Tok : Tokens) {
+ auto Macro = locateMacroAt(Tok, AST.getPreprocessor());
+ if (!Macro)
----------------
locateMacroAt doesn't check that the token is an identifier before trying to
fetch its text, maybe do that? either here or there...
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:115
+ },
+ // Misc
{
----------------
#define INNER 42
#define OUTER INNER
int answer = OUTER;
(whatever the behavior is, let's add a test for it)
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:115
+ },
+ // Misc
{
----------------
sammccall wrote:
> #define INNER 42
> #define OUTER INNER
>
> int answer = OUTER;
>
> (whatever the behavior is, let's add a test for it)
#define ANSWER 42
#define SQUARE(X) X * X
int sq = SQUARE(ANSWER);
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112447/new/
https://reviews.llvm.org/D112447
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
