[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/AST.cpp:353
     if (auto *AT = D->getType()->getContainedAutoType()) {
-      if (!AT->getDeducedType().isNull())
-        DeducedType = AT->getDeducedType();
+      DeducedType = AT->desugar();
     }
----------------
sammccall wrote:
> I added a comment to getDeducedType() - I think the intent here is that we return the AutoType itself if it's not deduced, right?
It is already the case for return types in `VisitFunctionDecl`, but not for types visited in in `VisitDeclaratorDecl`. I simply made it consistent. One could argue `getDeducedType` should return `None` when the type is not deduced, and I'd not argue against that. Though I have t admit this behavior makes things simpler for the hover feature.


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


================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+  } else {
+    CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+    if (D && D->isLambda())
----------------
sammccall wrote:
> 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.
We can probably use `printDefinition` for tag types and keep `getAsString` for non tag types. That would also make the definition close to what we get when hovering on an explicit type (as opposed to 'auto'). We'd probably need to set the scopes in `HoverInfo` as well, `printDefinition` does not print them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93227