[PATCH] D72500: [clangd] Show hover info for expressions

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:431
+
+// Generates hover info for string literals and evaluatable expressions.
+//  FIXME: Support hover for user-defined literals.
----------------
I'd really like to avoid *any literals here in the initial patch.

While there may be value in hover to see string length, I'm not sure the generic expr display code (which happens to show a type, which happens to include length) is the best way to get at this - seems pretty clunky.

(Similarly I think there's a case for UDLs, maybe showing numbers in different bases etc - but that's not the central point of this patch, so let's not try to work out exactly where the line is)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:438
+  // (apart from Expr), therefore this is a nasty blacklist.
+  if (llvm::isa<CharacterLiteral>(E) || llvm::isa<CompoundLiteralExpr>(E) ||
+      llvm::isa<CXXBoolLiteralExpr>(E) || llvm::isa<CXXNullPtrLiteralExpr>(E) ||
----------------
you could extract a isLiteral(StmtClass) function here and dispense with the second half of this comment, if you like.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:528
+  if (Name.empty()) {
+    // User is hovering over an expression, we only have Type and Value set.
+    // We'll have an output of the form:
----------------
I don't think it's a good idea to try to infer here what's being hovered on from which fields happen to be set, or to bail out assuming there's no documentation etc. (e.g it would be pretty reasonable for a UDL to return the doc of the operator as doc).

I'm also not sure abandoning the standard layout of properties is such a good idea, vs e.g.

---
### Binary expression

Type: `int`
Value = `4`

---

which would be consistent with the others (and simpler here).
We could always use "Expression" or "BinaryOperator" (from StmtClass) for now. Or maybe

---

### Expression: `int`

Value = `4`

---

Which still requires some special-casing, but less.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72500