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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the update, and thank you Sam for the suggestions!

In D148457#4307402 <https://reviews.llvm.org/D148457#4307402>, @sammccall wrote:

> A couple of ideas:
>
> - special-case alignof
> - (generalization) allow partial selection via macros that expand to a single token
> - (generalization) allow partial selection as long as it's of a single node - the common ancestor is partially selected and no children are

I was going to suggest a variant of the second option, where if a macro expands to a single token, the behaviour is the same as if the expanded token had been written in the source and that's what had been hovered over.

However, I don't have any strong concerns about the additional scenarios supported by the third option, so since this is what was implemented, let's go with it. We can always tweak this in the future if necessary.

---

Circling back to my original concern:

In D148457#4297847 <https://reviews.llvm.org/D148457#4297847>, @nridge wrote:

> the `Definition` field of the hover (which shows the tokens of the macro expansion) and the `Value` and `Type` fields (which show the value and type of a larger expression) could be out of sync and confusing

the original selection being "partial" vs. "complete" is just one part of this concern. Another part is the loop in `visitExprFromSelectionTree` that can choose a larger enclosing expression than the original selection (even if it's "complete"), so that we can get a `Definition` for a smaller expression but a `Value` for a larger one.

However, I discovered that this sort of discrepancy can already arise without macros, where the same loop means `Type` is given for a smaller expression and `Value` for a larger one. I filed https://github.com/clangd/clangd/issues/1622 for this with an example.

Since this is a pre-existing issue, I think it's fine not to worry about it in this patch. Perhaps in a follow-up patch we can find a solution that addresses both the macro and non-macro cases.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:471
+const SelectionTree::Node *
+visitExprFromSelectionTree(const SelectionTree::Node *N, const ASTContext &Ctx,
+                           llvm::function_ref<bool(const Expr *)> CB) {
----------------
Instead of generalizing `printExprValue` to take an arbitrary callback, can we modify the original function to return both the `Expr*` and its printed value (grouped in a pair or small struct)? And then our new call site can call `getType()` on the `Expr*`.

(I realize this would be a slight change in semantics as the callback for the macro codepath currently exits the loop as soon as we have a type... but I think there's value in keeping the behaviour consistent between the macro and non-macro cases.)


================
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
----------------
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.)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148457