[PATCH] D66928: [clangd] Collecting main file macro expansion locations in ParsedAST.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:107
 
+class CollectMainFileMacroExpansions : public PPCallbacks {
+  const SourceManager &SM;
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Maybe make this part of `CollectMainFileMacros`? It looks like a natural fit there and we won't need another instance of `PPCallbacks`.
> But `CollectMainFileMacros` is only used for `buildPreamble`. 
> I think this one needs to run in `ParsedAST::build`?
> 
> Is it safe to add a `CollectMainFileMacros in `ParsedAST::build`?
Ah, good point, LG then.
Could you put a small comment beside the class that explicitly mentions this?
It's obvious when reading through the whole code, but not obvious when peeking at different parts of it from a distance.

Something like
```
// CollectMainFileMacros and CollectMainFileMacroExpansions are two different classes because
// the latter is only used when building preamble and the former only when building the AST for the main file.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66928