[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/Macro.h:59
+
+    if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) {
+      MainFileMacros.push_back(
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Converting here is too early, could we keep this conversion in syntax highlighting code?
> > Keeping source locations here is enough.
> I'm not sure this is doable -- to get a range, we need the `SourceManager`, in the syntax highlighting context, we get the SourceManager from the `ParsedAST`, this `SourceManager` is used when building the main AST with preamble, I don't think we can use this  `SourceManager` to get the token range for macros in the preamble section?
Ah, yeah, good point. The source manager from preamble is not there anymore.
Ranges LG and this is what we used before.

Thanks for clearing this up.


================
Comment at: clang-tools-extra/clangd/Macro.h:67
+  bool InMainFile = true;
+  std::vector<MainFileMacro> &MainFileMacros;
+};
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Could we model in a way that avoids duplicating macro names on each occurrence?
> > We had `StringSet Names` and `vector<SourceLocation> Locations`, let's keep it in the same way.
> > 
> > We could group this into a struct to reduce boilerplate of transferring it around, obviously
> > ```
> > struct MainFileMacros {
> >   StringSet Names;
> >   vector<SourceLocation> Locations;
> > };
> > ```
> yes, we don't need the name and location reletionship in this patch, but we'd need this when implementing xrefs for macros. do you think we should keep the same way, and do the change when we start implementing xrefs for macros?
Cross-references have to be more fine-grained and having only a name is not enough. Let's address this separately when/if we actually need this in xrefs.
In particular, we want to handle macro re-definitions:
```
#define FOO 123 // #1
int a = FOO; // a reference to #1

#define FOO 234 // #2
int b = FOO; // a reference to #2
```
Current model will merge these two together as both names are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496