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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
