[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Sam McCall via Phabricator via cfe-commits]

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