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

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz marked 5 inline comments as done.
daiyousei-qz added a comment.

I have also moved the expansion out of the location checking branch. I think it's to find out if a macro actually has a definition which isn't always the case especially for special macros like `__FILE__`.

Here's the current presentation.
F23848502: image.png <https://reviews.llvm.org/F23848502>

F23848497: image.png <https://reviews.llvm.org/F23848497>

F23848508: image.png <https://reviews.llvm.org/F23848508>



================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:490
+         HI.Definition = "#define MACRO 41";
+         HI.MacroExpansion = "41";
+       }},
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082