Page MenuHomePhabricator

[clangd] Show hover info for expressions
ClosedPublic

Authored by kadircet on Jan 10 2020, 4:13 AM.

Details

Summary

This currently populates only the Name with the expression's type and
Value if expression is evaluatable.

Fixes https://github.com/clangd/clangd/issues/56

Diff Detail

Event Timeline

kadircet created this revision.Jan 10 2020, 4:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 4:13 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Basing this on the hover for the type doesn't seem right. e.g. int should be the Type rather than the Name.

Rather than printing the value if evaluable, I think we should only show the hover if evaluable. There's a cost to showing it and the value of just the type doesn't seem clearly high enough.

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

lh123 added a subscriber: lh123.EditedJan 11 2020, 8:13 PM

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

Hovering over IntegerLiteral/FloatingLiteral may be useless, but I think it's useful when hovering over StringLiteral/UserDefinedLiteral/CXXNullPtrLiteralExpr ....

  • "hello" -> char [6].
  • nullptr -> std::nullptr_t.
  • 1i -> std::complex<double>.

Basing this on the hover for the type doesn't seem right. e.g. int should be the Type rather than the Name.

Rather than printing the value if evaluable, I think we should only show the hover if evaluable. There's a cost to showing it and the value of just the type doesn't seem clearly high enough.

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

agreed.

I think we should avoid triggering for literals. Maybe some exceptions, but a hover saying that 0 is an int with value 0 seems silly.

Hovering over IntegerLiteral/FloatingLiteral may be useless, but I think it's useful when hovering over StringLiteral/UserDefinedLiteral/CXXNullPtrLiteralExpr ....

  • "hello" -> char [6].
  • nullptr -> std::nullptr_t.
  • 1i -> std::complex<double>.

I believe all but the first case seems redundant. So I am only keeping the StringLiterals and dropping the rest.

kadircet updated this revision to Diff 237649.Jan 13 2020, 6:01 AM
  • Ignore literals
kadircet retitled this revision from [clangd] Show hower info for expressions to [clangd] Show hover info for expressions.Jan 13 2020, 6:07 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

kadircet updated this revision to Diff 237660.Jan 13 2020, 6:28 AM
  • Update presentation for expressions, which doesn't have Name field set.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

sammccall added inline comments.Jan 14 2020, 2:59 AM
clang-tools-extra/clangd/Hover.cpp
421

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)

428

you could extract a isLiteral(StmtClass) function here and dispense with the second half of this comment, if you like.

522

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.

kadircet updated this revision to Diff 238009.Jan 14 2020, 9:43 AM
kadircet marked 3 inline comments as done.
  • Populate Name for expressions
  • Don't special case StringLiterals

Unit tests: pass. 61794 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

sammccall accepted this revision.Jan 14 2020, 10:09 AM

LG!

clang-tools-extra/clangd/Hover.cpp
519

nit: this is just "expression"

This revision is now accepted and ready to land.Jan 14 2020, 10:09 AM
This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
kadircet added inline comments.Jan 15 2020, 6:36 AM
clang-tools-extra/clangd/Hover.cpp
519

not yet, it will be after D72623 lands.