[PATCH] D93227: [clangd] Smarter hover on auto and decltype

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Agree with all the conclusions you've come to here. Main two issues are:

- the new traversal added to patch up some cases isn't the right approach IMO.
- some accidental regressions

I'd also like to distinguish decltype(auto) from other decltypes, but it's a problem with our existing helpers so let's not try that in this patch.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+    CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+    if (D && D->isLambda())
----------------
You've rewritten this logic compared to the old `getHoverContents(QualType)`, and I think there are regressions:
 - We've dropped class documentation (see e.g. ac3f9e48421712168884d59cbfe8b294dd76a19b, this is visible in the tests)
 - we're no longer using printName() to print tag-decls, which I expect changes e.g. the printing of anyonymous structs which is special-cased in that function (we have tests for this but probably not in combination with auto)
 - we're no longer using the printing-policy to print non-tag types, which will lead to some random changes

I don't see a reason for these changes, can we revert them?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:631
+// getDeducedType are handled.
+class ExtraAutoTypeHoverVisitor
+    : public RecursiveASTVisitor<ExtraAutoTypeHoverVisitor> {
----------------
This functionality (reporting the `auto` type in structured bindings, and not reporting non-deduced uses of auto) belongs in the getDeducedType() helper rather than as a layer on top.
(Especially because it has to be done via an AST traversal rather than SelectionTree)

I'd suggest leaving it out of this patch to get this the original change landed quickly - this seems like a mostly-unrelated enhancement. But up to you.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:907
+
+      HI->Name = tok::getTokenName(Tok.kind());
+      HighlightRange = Tok.range(SM).toCharRange(SM);
----------------
decltype(auto) and decltype(expr) are fairly different things and ultimately we should be displaying them differently I think ("decltype(auto)" vs "decltype(...)").

Unfortunately it's awkward because our getDeducedType helper handles both at the moment (and so is misnamed, because decltype(expr) isn't deduced at all).

Can you add `// FIXME: distinguish decltype(auto) vs decltype(expr)` and I'll do some refactoring later?


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:999
+      "decltype(au^to) x = 0;",
+      R"cpp(// Lambda auto parameter. Nothing (Not useful).
+            auto lamb = [](a^uto){};
----------------
(not convinced this is fundamentally not useful - the fact that it's a template parameter means it's probably worth having a hover card for it at some point. But I agree with suppressing it for now)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1894
+            HI.Definition = "int &&";
+          }},
       {
----------------
can we have a decltype test where the result is dependent?
e.g..

```
template <typename T>
struct X {
  using Y = ^decltype(T::Z);
};
```

should yield a definition of "<dependent type>" rather than "/* not deduced */".
(Since regular decltype() is not a deduced type)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227