- store all macro references in the ParsedAST, which also helps to implement find references for macros.
- unify the two variants of CollectMainFileMacros.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 38052 Build 38051: arc lint + arc unit
Event Timeline
Sorry for the delay
clang-tools-extra/clangd/Macro.h | ||
---|---|---|
59 | Converting here is too early, could we keep this conversion in syntax highlighting code? | |
67 | Could we model in a way that avoids duplicating macro names on each occurrence? We could group this into a struct to reduce boilerplate of transferring it around, obviously struct MainFileMacros { StringSet Names; vector<SourceLocation> Locations; }; |
clang-tools-extra/clangd/Macro.h | ||
---|---|---|
59 | 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? | |
67 | 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? |
clang-tools-extra/clangd/Macro.h | ||
---|---|---|
59 | Ah, yeah, good point. The source manager from preamble is not there anymore. Thanks for clearing this up. | |
67 | 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. #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. |
clang-tools-extra/clangd/Macro.h | ||
---|---|---|
67 | good point, agreed. I think we probably use the symbol id for macros, but it is out-scope of this patch. |
LGTM, thanks.
A ton of small NITs too, but they're all small details.
clang-tools-extra/clangd/Macro.h | ||
---|---|---|
2 | NIT: Maybe rename to CollectMacros.h? Macro.h looks a bit too generic and may start being a place for more helpers. However, the current file is pretty well-structured. | |
24 | Could you add a comment that we have to convert to Range early because source manager from preamble is not available when we build the AST? | |
27 | NIT: a typo: in the main file. It is used ... | |
29 | NIT: use /// to enable doxygen comments | |
66 | NIT: It's probably safer to set 'no' by default. OTOH, FileChanged should be called first in any case, so it does not matter. | |
clang-tools-extra/clangd/ParsedAST.h | ||
104 | NIT: rename parameter to Macros | |
124–125 | NIT: change definitions/expansions to definitions and expansion | |
clang-tools-extra/clangd/Preamble.cpp | ||
36 | NIT: rename to takeMacros | |
76 | NIT: add const to SourceManager too, for consistency | |
clang-tools-extra/clangd/Preamble.h | ||
61 | There seems to be no reason for moving constructor to the bottom of the class. Maybe keep as is? |
NIT: Maybe rename to CollectMacros.h?
Macro.h looks a bit too generic and may start being a place for more helpers. However, the current file is pretty well-structured.