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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:353
     if (auto *AT = D->getType()->getContainedAutoType()) {
-      if (!AT->getDeducedType().isNull())
-        DeducedType = AT->getDeducedType();
+      DeducedType = AT->desugar();
     }
----------------
I added a comment to getDeducedType() - I think the intent here is that we return the AutoType itself if it's not deduced, right?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:591
+                                                   ASTContext &ASTCtx,
+                                                   const SymbolIndex *Index) {
+  QualType OriginThisType = CTE->getType()->getPointeeType();
----------------
I dropped this unused param


================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+    CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+    if (D && D->isLambda())
----------------
sammccall wrote:
> 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 :-)
Hmm, it looks like this wasn't done. It's now printing e.g. `class vector<class Foo>` which seems more confusing than either `class vector<Foo>` or `vector<Foo>` (even if we lose some highlighting). I'll send a followup with proposed changes here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227