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