[PATCH] D148457: [clangd] Support macro evaluation on hover

[Younan Zhang via Phabricator via cfe-commits]

zyounan added a comment.

Thank you for the opinions. I've updated and please take a look.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:705
+
+  // If macro expands to one single token, rule out punctuator or digraph.
+  // E.g., for the case `array L_BRACKET 42 R_BRACKET;` where L_BRACKET and
----------------
nridge wrote:
> This check for punctuators actually makes the behaviour **more strict** for macros than for non-macros in some cases.
> 
> For example:
> 
> ```
> #define PLUS +
> 
> constexpr int A = 2 + 3;  // hover over `+` shows `Value = 5`
> constexpr int B = 2 PLUS 3;  // hover over `PLUS` does not show `Value`
> ```
> 
> I don't think this is a particularly important use case (it's a bit of a hack to get the expression's value this way, much better would be to select the entire expression you want to evaluate in the editor, but unfortunately LSP only sends a single position in `textDocument/hover`, not a full range...), but perhaps we could consider relaxing this in the future. (I'm thinking, if we switched to "allow partial selection via macros that expand to a single token", we could probably drop this condition.)
Thanks for clarifying this intention (for any bug hunter in the future).
> if we switched to "allow partial selection via macros that expand to a single token", we could probably drop this condition.

I'm afraid we can't. For the array case in the test,
```
vector left_b^racket 3 right_b^racket;
// left_bracket -> [
// right_bracket -> ]
```
the associated selection tree is,
```
 TranslationUnitDecl 
   FunctionDecl void function()
     CompoundStmt { …
      .ArraySubscriptExpr vector[3]
```
(`function` is the wrapper that facilitates writing single expression, doesn't matter here.)
It expands to one single token and is partially selected, which exactly matches the strategy. If we do allow evaluation, we'd get a value/type on `[` or `]`. Yes, it is a contrived and weird use case, but for now I think it's fine to keep it unevaluated to avoid issue aforementioned.

(But I'm willing to remove this restriction if you prefer a relaxed strategy.)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3832
+          };
+          Alig$3^nOf(Y);
+        )cpp",
----------------
nridge wrote:
> I guess with this style of test you can simplify the `$3^` to `^`
Aha, that's a typo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148457