[PATCH] D112447: [clangd] IncludeCleaner: Support macros

[Sam McCall via Phabricator via cfe-commits]

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