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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! I think this is in pretty good shape, remaining stuff is up to you.

Let me know if/when you want me to land this!



================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+    CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+    if (D && D->isLambda())
----------------
qchateau wrote:
> 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`.
> should it work when auto is a pointer or a ref?

I think no need for now, I just want to avoid the regression.
(Nice to have, but at least in the codebases I work in, `auto` is much more common than `decltype(auto)` so it's rarely a reference, and pointers are idiomatically written `auto*`)

> printName will print C instead of class C even if I hack around and give it the right PrintingPolicy.

Yeah... it's not supposed to, though the extra clarity is valuable here. You could just hack around this and prepend the TagTypeKind yourself :-)


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:1717
             struct Bar {};
+            // auto function return with trailing type
             ^[[auto]] test() -> decltype(Bar()) {
----------------
can you restore the comments to the original position above the struct, so we test that the documentation comment is included?


================
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){};
----------------
qchateau wrote:
> 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 ?
Sorry, I think we're talking about different examples.

(I agree decltype(auto) should ideally be a single thing and we should support hover on all of it, no need to address in this patch, FIXME would be nice).

But I was talking about the lambda auto parameter, where your comment says "not useful". Anyway, nothing to do here either except maybe soften the comment to "not supported at the moment".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227