sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1187
- if (!Definition.empty()) {
+ if (!Definition.empty() || !MacroExpansion.empty()) {
Output.addRuler();
----------------
Sorry, I think I wasn't clear about what I was asking for...
Rather than merging `HoverInfo::MacroExpansion` and `HoverInfo::Definition`
during `present()`, I'd really like to eliminate `HoverInfo::MacroExpansion`
entirely, and have `HoverInfo::Definition` be:
```
#define FOO BAR
// Expands to
BAR
```
This isn't completely consistent with the way scope/access are handled, but
those are somewhat more generic features and I'd rather avoid adding new
HoverInfo members specific to macros.
(The final presentation does look good, so this is just about the intermediate
HoverInfo struct)
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:490
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+ }},
----------------
daiyousei-qz wrote:
> sammccall wrote:
> > this is redundant with the definition, ideally I think we'd omit one (the
> > output is pretty heavyweight).
> >
> > It's *almost* correct to omit the expansion for object-like macros, the
> > problem with this is:
> > ```
> > #define INDIRECT DOUBLE(y)
> > #define DOUBLE(X) X##X
> > int INDIRECT; // we *should* show expansion here
> > ```
> >
> > Still, this is probably rare enough that it's better to not show expansions
> > for object-like macros on balance?
> >
> > (It would be possible to actually check for nested expansions but I doubt
> > it's worth the complexity, certainly not in this patch)
> I suppose if we don't have good way to detect nested object-like macro, just
> leave both definition and expansion is a better idea since people could
> simply ignore the redundant part. Though I don't have a strong opinion on
> this and could change that if you really want.
OK, I also don't feel strongly, so let's go with what you have now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127082/new/
https://reviews.llvm.org/D127082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
