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

[Nathan Ridge via Phabricator via cfe-commits]

nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM with one final nit (the other comment is just food for future thought)



================
Comment at: clang-tools-extra/clangd/Hover.cpp:503
+
+std::optional<std::string> printExprValue(const SelectionTree::Node *N,
+                                          const ASTContext &Ctx) {
----------------
nit: just inline this into its only call site as `doPrintExprValue(N, Ctx).PrintedValue` (and then we can call the function `printExprValue` instead of `doPrintExprValue`)


================
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
----------------
zyounan wrote:
> 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.)
> If we do allow evaluation, we'd get a value/type on [ or ].

Out of curiosity, I tried commenting out this condition locally, and the `left_bracket` test case still passed for me.

The reason is that in that test case, the ArraySubscriptExpr cannot be evaluated (we don't know what the array values are).

In a modified version of the testcase where the array values are known, but macros are not used:

```
int main() {
  constexpr int vector[3] = {1, 2, 3};
  vector [ 2 ];
}
```

hovering over the `[` or `]` does work (in clangd today, no patch needed).

(But maybe a hover in this situation is more confusing than useful, and it would be better if it didn't work?)

Anyways, we can discuss this further in follow-up issues like https://github.com/clangd/clangd/issues/1622, I think this is good enough for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148457