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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Macro.h:59
+
+    if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) {
+      MainFileMacros.push_back(
----------------
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?


================
Comment at: clang-tools-extra/clangd/Macro.h:67
+  bool InMainFile = true;
+  std::vector<MainFileMacro> &MainFileMacros;
+};
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67496