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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev 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 =
----------------
sammccall wrote:
> 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?
Hmm, this doesn't really work for "unmaterialized" macros, i.e. `ParsedAST::getMacros().Names` doesn't have `FOO` in this case:

```
// foo.h
#define FOO

// foo.cpp
#define BAR FOO
```

Is that a bug in `CollectMainFileMacros` we want to fix or is this intended behavior and we'd need to iterate through the tokens?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112447