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

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+    CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+    if (D && D->isLambda())
----------------
sammccall wrote:
> 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?
- I can re-add class documentation, but should it work when `auto` is a pointer or a ref ? In that case, I'll need something like your `unwrapType` of https://reviews.llvm.org/D93314
- `printName` will print `C` instead of `class C` even if I hack around and give it the right `PrintingPolicy`. The problem is that IDE (at least VSCode) does not know what `C` is and cannot color it, which looks a nice less nice. Up to you !
- I can re-add the printing policy for non-tag types, and add it for tag-types if we chose not to use `printName`.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:631
+// getDeducedType are handled.
+class ExtraAutoTypeHoverVisitor
+    : public RecursiveASTVisitor<ExtraAutoTypeHoverVisitor> {
----------------
sammccall wrote:
> 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.
I'll remove this. I'll leave out the case of structured bindings for arrays (which is really weird and specific), but I guess I'll fix`getDeducedType` to return the undeduced `QualType` instead of `None` (which it already does but only for return types).


================
Comment at: clang-tools-extra/clangd/Hover.cpp:907
+
+      HI->Name = tok::getTokenName(Tok.kind());
+      HighlightRange = Tok.range(SM).toCharRange(SM);
----------------
sammccall wrote:
> 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?
Sure, I'll add the comment. I'll leave that refactoring to you, I'm not quite sure how you intent to achieve it.


================
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){};
----------------
sammccall wrote:
> (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)
As a user I'd prefer the hover to work over the whole `decltype(auto)` expression. But that does not seem quite compatible with the way tokens are parsed.

Are you suggesting I remove the test case or should I add a `FIXME` comment ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227