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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
