[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0474fe465d8f: [clangd] Print underlying type for decltypes 
in hover (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1514,6 +1514,56 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) c;
+  decltype(c) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(c) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  const decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "const decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  auto [[f^oo]](decltype(a) x) -> decltype(a) { return 0; })cpp",
+  [](HoverInfo ) {
+HI.Definition = "auto foo(decltype(a) x) -> decltype(a)";
+HI.Kind = index::SymbolKind::Function;
+HI.NamespaceScope = "";
+HI.Name = "foo";
+// FIXME: Handle composite types with decltype with a printing
+// policy.
+HI.Type = "auto (decltype(a)) -> decltype(a)";
+HI.ReturnType = "int";
+HI.Parameters = {
+{std::string("int"), std::string("x"), llvm::None}};
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
@@ -1542,6 +1592,7 @@
 Expected.SymRange = T.range();
 Case.ExpectedBuilder(Expected);
 
+SCOPED_TRACE(H->present().asPlainText());
 EXPECT_EQ(H->NamespaceScope, Expected.NamespaceScope);
 EXPECT_EQ(H->LocalScope, Expected.LocalScope);
 EXPECT_EQ(H->Name, Expected.Name);
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -123,6 +123,15 @@
   }
 }
 
+std::string printType(QualType QT, const PrintingPolicy ) {
+  // TypePrinter doesn't resolve decltypes, so resolve them here.
+  // FIXME: This doesn't handle composite types that contain a decltype in them.
+  // We should rather have a printing policy for that.
+  while (const auto *DT = QT->getAs())
+QT = DT->getUnderlyingType();
+  return QT.getAsString(Policy);
+}
+
 std::vector
 fetchTemplateParameters(const TemplateParameterList *Params,
 const PrintingPolicy ) {
@@ -131,8 +140,7 @@
 
   for (const Decl *Param : *Params) {
 HoverInfo::Param P;
-P.Type.emplace();
-if (const auto TTP = dyn_cast(Param)) {
+if (const auto *TTP = dyn_cast(Param)) {
   P.Type = TTP->wasDeclaredWithTypename() ? "typename" : "class";
   if (TTP->isParameterPack())
 *P.Type += "...";
@@ -141,21 +149,21 @@
 P.Name = TTP->getNameAsString();
   if (TTP->hasDefaultArgument())
 P.Default = TTP->getDefaultArgument().getAsString(PP);
-} else if (const auto NTTP = dyn_cast(Param)) {
+} else if (const auto *NTTP = dyn_cast(Param)) {
   if (IdentifierInfo *II = NTTP->getIdentifier())
 P.Name = II->getName().str();
 
-  llvm::raw_string_ostream Out(*P.Type);
-  NTTP->getType().print(Out, PP);
+  P.Type = printType(NTTP->getType(), PP);
   if (NTTP->isParameterPack())
-Out << "...";
+*P.Type += "...";
 
   if (NTTP->hasDefaultArgument()) {
 P.Default.emplace();
 llvm::raw_string_ostream Out(*P.Default);
 NTTP->getDefaultArgument()->printPretty(Out, nullptr, PP);
   }
-} else if (const auto TTPD = dyn_cast(Param)) {
+} else if (const auto *TTPD = dyn_cast(Param)) {
+  P.Type.emplace();
   llvm::raw_string_ostream OS(*P.Type);
   OS << "template <";
   printParams(OS,
@@ 

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Still LG


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61913 tests passed, 0 failed 
and 783 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 238474.
kadircet added a comment.

- Handle return and parameter types as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1514,6 +1514,56 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) c;
+  decltype(c) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(c) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  const decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "const decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  auto [[f^oo]](decltype(a) x) -> decltype(a) { return 0; })cpp",
+  [](HoverInfo ) {
+HI.Definition = "auto foo(decltype(a) x) -> decltype(a)";
+HI.Kind = index::SymbolKind::Function;
+HI.NamespaceScope = "";
+HI.Name = "foo";
+// FIXME: Handle composite types with decltype with a printing
+// policy.
+HI.Type = "auto (decltype(a)) -> decltype(a)";
+HI.ReturnType = "int";
+HI.Parameters = {
+{std::string("int"), std::string("x"), llvm::None}};
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
@@ -1542,6 +1592,7 @@
 Expected.SymRange = T.range();
 Case.ExpectedBuilder(Expected);
 
+SCOPED_TRACE(H->present().asPlainText());
 EXPECT_EQ(H->NamespaceScope, Expected.NamespaceScope);
 EXPECT_EQ(H->LocalScope, Expected.LocalScope);
 EXPECT_EQ(H->Name, Expected.Name);
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -123,6 +123,15 @@
   }
 }
 
+std::string printType(QualType QT, const PrintingPolicy ) {
+  // TypePrinter doesn't resolve decltypes, so resolve them here.
+  // FIXME: This doesn't handle composite types that contain a decltype in them.
+  // We should rather have a printing policy for that.
+  while (const auto *DT = QT->getAs())
+QT = DT->getUnderlyingType();
+  return QT.getAsString(Policy);
+}
+
 std::vector
 fetchTemplateParameters(const TemplateParameterList *Params,
 const PrintingPolicy ) {
@@ -131,8 +140,7 @@
 
   for (const Decl *Param : *Params) {
 HoverInfo::Param P;
-P.Type.emplace();
-if (const auto TTP = dyn_cast(Param)) {
+if (const auto *TTP = dyn_cast(Param)) {
   P.Type = TTP->wasDeclaredWithTypename() ? "typename" : "class";
   if (TTP->isParameterPack())
 *P.Type += "...";
@@ -141,21 +149,21 @@
 P.Name = TTP->getNameAsString();
   if (TTP->hasDefaultArgument())
 P.Default = TTP->getDefaultArgument().getAsString(PP);
-} else if (const auto NTTP = dyn_cast(Param)) {
+} else if (const auto *NTTP = dyn_cast(Param)) {
   if (IdentifierInfo *II = NTTP->getIdentifier())
 P.Name = II->getName().str();
 
-  llvm::raw_string_ostream Out(*P.Type);
-  NTTP->getType().print(Out, PP);
+  P.Type = printType(NTTP->getType(), PP);
   if (NTTP->isParameterPack())
-Out << "...";
+*P.Type += "...";
 
   if (NTTP->hasDefaultArgument()) {
 P.Default.emplace();
 llvm::raw_string_ostream Out(*P.Default);
 NTTP->getDefaultArgument()->printPretty(Out, nullptr, PP);
   }
-} else if (const auto TTPD = dyn_cast(Param)) {
+} else if (const auto *TTPD = dyn_cast(Param)) {
+  P.Type.emplace();
   llvm::raw_string_ostream OS(*P.Type);
   OS << "template <";
   printParams(OS,
@@ -241,7 +249,7 @@
 HI.Parameters->emplace_back();
 auto  = 

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61885 tests passed, 0 failed 
and 782 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 238245.
kadircet added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1534,6 +1534,53 @@
 HI.Type = "unsigned long";
 HI.Value = "1";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) c;
+  decltype(c) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(c) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  const decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "const decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  auto [[f^oo]]() -> decltype(a) { return 0; })cpp",
+  [](HoverInfo ) {
+HI.Definition = "auto foo() -> decltype(a)";
+HI.Kind = index::SymbolKind::Function;
+HI.NamespaceScope = "";
+HI.Name = "foo";
+HI.Type = "auto () -> decltype(a)";
+HI.ReturnType = "int";
+HI.Parameters.emplace();
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -265,12 +265,17 @@
   } else if (llvm::isa(FD)) {
 HI.ReturnType = "void";
   } else {
-HI.ReturnType = FD->getReturnType().getAsString(Policy);
-
-QualType FunctionType = FD->getType();
+QualType QT = FD->getReturnType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are going
+// to include spelling "decltype(X)" in `HoverInfo::Definition` anyway.
+while (auto *DT = QT->getAs())
+  QT = DT->getUnderlyingType();
+HI.ReturnType = QT.getAsString(Policy);
+
+QT = FD->getType();
 if (const VarDecl *VD = llvm::dyn_cast(D)) // Lambdas
-  FunctionType = VD->getType().getDesugaredType(D->getASTContext());
-HI.Type = FunctionType.getAsString(Policy);
+  QT = VD->getType().getDesugaredType(D->getASTContext());
+HI.Type = QT.getAsString(Policy);
   }
   // FIXME: handle variadics.
 }
@@ -352,8 +357,14 @@
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
 fillFunctionTypeAndParams(HI, D, FD, Policy);
-  else if (const auto *VD = dyn_cast(D))
-HI.Type = VD->getType().getAsString(Policy);
+  else if (const auto *VD = dyn_cast(D)) {
+QualType QT = VD->getType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are going
+// to include spelling "decltype(X)" in `HoverInfo::Definition` anyway.
+while (auto *DT = QT->getAs())
+  QT = DT->getUnderlyingType();
+HI.Type = QT.getAsString(Policy);
+  }
 
   // Fill in value with evaluated initializer if possible.
   if (const auto *Var = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72498#1817070 , @sammccall wrote:

> No, I think printing *both* is at least somewhat likely to be too verbose, 
> especially since the previous release showed no types at all.
>  And we're out of time to iterate on the behavior and presentation for this 
> cycle. I think we should do something more conservative and then experiment 
> in the next release cycle.


Sorry, I didn't mention it explicitly in my previous post but I was also 
planning to make it a separate patch, independent of this one. I am also OK 
with landing it after branch cut.

As for this patch, I still believe there's a lot of value in either:

- handling non-composite type cases, as proposed in current change
- handling of both basic and composite type cases in typeprinter through a 
printing policy

I am happy to follow any of those, though the second might be more disruptive 
before the branch cut.
So I would rather also introduce handling of decltypes to parameter and return 
types and land this change.
Because even though it might be messy to hover over nested templated types, I 
still believe it will most likely save user time in the general case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D72498#1816957 , @kadircet wrote:

> In D72498#1816785 , @ilya-biryukov 
> wrote:
>
> > In D72498#1816424 , @lh123 wrote:
> >
> > > Currently, I think that in most cases, showing both expanded (canonical) 
> > > and spelled types is sufficient.
> > >
> > > > This has been used in ycmd for ~4 years without complaint. 
> > > > https://github.com/clangd/clangd/issues/58#issuecomment-507800970
> >
> >
> > That actually doesn't look bad. Maybe let's try doing that and see whether 
> > we'll get negative feedback?
> >  That seems to give useful information in **all** cases, so at least it'll 
> > cover all use-cases even it's more verbose.
> >
> > What do others think?
>
>
> SGTM, happy to update all types in `HoverInfo` to contain both a 
> `pretty-printed` and `canonical` version. Where pretty-printed would just 
> means desugared, so keywords like auto/decltype would've
>  been stripped away and it would be the type as written in all other cases, 
> while canonical would refer to `clang canonical` with all of the type aliases 
> etc. resolved. Ofc.
>  Does that SG to you as well @sammccall ?


No, I think printing *both* is at least somewhat likely to be too verbose, 
especially since the previous release showed no types at all.
And we're out of time to iterate on the behavior and presentation for this 
cycle. I think we should do something more conservative and then experiment in 
the next release cycle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72498#1816785 , @ilya-biryukov 
wrote:

> In D72498#1816424 , @lh123 wrote:
>
> > Currently, I think that in most cases, showing both expanded (canonical) 
> > and spelled types is sufficient.
> >
> > > This has been used in ycmd for ~4 years without complaint. 
> > > https://github.com/clangd/clangd/issues/58#issuecomment-507800970
>
>
> That actually doesn't look bad. Maybe let's try doing that and see whether 
> we'll get negative feedback?
>  That seems to give useful information in **all** cases, so at least it'll 
> cover all use-cases even it's more verbose.
>
> What do others think?


SGTM, happy to update all types in `HoverInfo` to contain both a 
`pretty-printed` and `canonical` version. Where pretty-printed would just means 
desugared, so keywords like auto/decltype would've
been stripped away and it would be the type as written in all other cases, 
while canonical would refer to `clang canonical` with all of the type aliases 
etc. resolved. Ofc.
Does that SG to you as well @sammccall ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1816424 , @lh123 wrote:

> Currently, I think that in most cases, showing both expanded (canonical) and 
> spelled types is sufficient.
>
> > This has been used in ycmd for ~4 years without complaint. 
> > https://github.com/clangd/clangd/issues/58#issuecomment-507800970


That actually doesn't look bad. Maybe let's try doing that and see whether 
we'll get negative feedback?
That seems to give useful information in **all** cases, so at least it'll cover 
all use-cases even it's more verbose.

What do others think?

In D72498#1816668 , @kadircet wrote:

> In D72498#1813989 , @ilya-biryukov 
> wrote:
>
> > In D72498#1813962 , @sammccall 
> > wrote:
> >
> > >
> >
> >
> > I tend to disagree here. decltype is normally the last resort, so whatever 
> > it produces is probably super-obscure, would even expect it to be not 
> > representable in C++ in many cases.
>
>
> I was rather talking about the obscurity of the expression inside decltype vs 
> the typedef alias. I believe it is a lot harder to make any assumptions on 
> `decltype(callback)` compared to `IntMap` without seeing the underlying type.


Point taken, although I bet we could come up with examples of obscure results 
in both cases.

>> Would definitely be helpful. If you feel we have some room in hover, I would 
>> love to have that. But there's a balance to be made, see Sam's comments 
>> about canonical types being obscure. I agree on 50% of the cases :-)
> 
> I think this should be OK to spend some space, as it will only show up when 
> needed. I believe `better` printing of canonical types is a different problem 
> we should definitely solve.

Totally agree, improving printing of STL types would be huge, no matter whether 
they're canonical or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D72498#1813989 , @ilya-biryukov 
wrote:

> In D72498#1813962 , @sammccall wrote:
>
> > It's particularly unclear to me why typeprinter descends into auto but 
> > prints decltype, but Kadir says that seems to be intentional.
>
>
> Also don't see why they choose to have this inconsistency and I haven't seen 
> any indication it's not a coincidence.
>  @kadircet, why do you think it's intentional? Should we put some comments 
> there?


I was saying that because `TypePrinter` deliberately prints `decltype(` and 
then the expression.
I suppose we could make this a printing policy option to print either the 
underlying expr or type.

> I tend to disagree here. decltype is normally the last resort, so whatever it 
> produces is probably super-obscure, would even expect it to be not 
> representable in C++ in many cases.

I was rather talking about the obscurity of the expression inside decltype vs 
the typedef alias. I believe it is a lot harder to make any assumptions on 
`decltype(callback)` compared to `IntMap` without seeing the underlying type.

> Would definitely be helpful. If you feel we have some room in hover, I would 
> love to have that. But there's a balance to be made, see Sam's comments about 
> canonical types being obscure. I agree on 50% of the cases :-)

I think this should be OK to spend some space, as it will only show up when 
needed. I believe `better` printing of canonical types is a different problem 
we should definitely solve.

@sammccall wrote:

> It's a contrived example that makes clangd look silly (why decltype(a) 
> instead of int?) but also the user look silly (why hover the variable rather 
> than the decltype?).


Right, the user can definitely hover over the decltype, but this goes against 
our objectives. I believe with hover we are trying to reduce number of jumps a 
user needs to take for acquiring some info,
and requiring them to jump to the definition of symbol for figuring out the 
type doesn't seem right.

> But note that this patch doesn't handle the important use case of decltype as 
> return type, really!
>  imagine cout << sum('c', 4), and you want to know what type the function 
> call has.
>  The natural thing is to hover over the function call, you'll see "function 
> sum -> decltype(t1 + t2)" which is indeed pretty useless.
>  But this patch doesn't fix that behaviour, it only handles types of 
> declarations.

Yes, but this was an oversight rather than an explicit decision. I believe we 
should definitely do the same for return type and even parameter types.

> @ilya-biryukov @kadircet what do you think about unwrapping decltype only 
> when it's a return value (optional: of a function whose leading return type 
> is auto) to narrowly catch this idiom?

I wouldn't special case that behavior as it is equally useful for ValueDecls, 
since we are trying to get rid of the extra jump as mentioned above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D72498#1816339 , @sammccall wrote:

> In D72498#1816244 , @ilya-biryukov 
> wrote:
>
> > In D72498#1815500 , @lh123 wrote:
> >
> > > - hover over the `front` , you'll see "instance-method `front` → 
> > > `std::vector >::reference`".
> > > - hover over the `push_back`, you'll see "`std::vector > > std::allocator >::value_type && __x`".
> >
> >
> > These look terrible and are the great examples where showing canonical 
> > types results in better output than canonical types.
> >  I wonder why we add `std::vector>::` in the 
> > first place, I believe the standard library uses `value_type` in the 
> > declaration. Showing `value_type` is not great, but at least that doesn't 
> > uglify what was written in the code in the first place.
> >  FWIW, I think the perfect output in those cases would be `int (aka 
> > value_type)`
>
>
> Indeed. Another illustrative example, the return type of 
> `vector::at()` - we'd probably want `int64&` here, rather than 
> `vector<...>::reference` or `unsigned long long`/`unsigned long` depending on 
> platform.


Currently, I think that in most cases, showing both expanded (canonical) and 
spelled types is sufficient.

> This has been used in ycmd for ~4 years without complaint. 
> https://github.com/clangd/clangd/issues/58#issuecomment-507800970




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D72498#1816244 , @ilya-biryukov 
wrote:

> In D72498#1815500 , @lh123 wrote:
>
> > - hover over the `front` , you'll see "instance-method `front` → 
> > `std::vector >::reference`".
> > - hover over the `push_back`, you'll see "`std::vector > std::allocator >::value_type && __x`".
>
>
> These look terrible and are the great examples where showing canonical types 
> results in better output than canonical types.
>  I wonder why we add `std::vector>::` in the 
> first place, I believe the standard library uses `value_type` in the 
> declaration. Showing `value_type` is not great, but at least that doesn't 
> uglify what was written in the code in the first place.
>  FWIW, I think the perfect output in those cases would be `int (aka 
> value_type)`


Indeed. Another illustrative example, the return type of 
`vector::at()` - we'd probably want `int64&` here, rather than 
`vector<...>::reference` or `unsigned long long`/`unsigned long` depending on 
platform.

It seems like:

- unwrapping nothing isn't ideal
- unwrapping everything isn't ideal
- brevity might be a good heuristic, but unclear
- there's value sometimes in showing multiple forms, unclear exactly when

(Machine learning time? Mostly joking...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1815500 , @lh123 wrote:

> - hover over the `front` , you'll see "instance-method `front` → 
> `std::vector >::reference`".
> - hover over the `push_back`, you'll see "`std::vector std::allocator >::value_type && __x`".


These look terrible and are the great examples where showing canonical types 
results in better output than canonical types.
I wonder why we add `std::vector>::` in the 
first place, I believe the standard library uses `value_type` in the 
declaration. Showing `value_type` is not great, but at least that doesn't 
uglify what was written in the code in the first place.
FWIW, I think the perfect output in those cases would be `int (aka value_type)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1814366 , @sammccall wrote:

> @ilya-biryukov @kadircet what do you think about unwrapping decltype only 
> when it's a return value (optional: of a function whose leading return type 
> is auto) to narrowly catch this idiom?


If we feel it's useful in the function return type, it's probably also useful 
in other template contexts:
E.g.

  template 
  struct X {
typedef decltype(T() + T()) add_result_type;
  };
  
  X::^add_result_type y;

And I don't think it's used in practice in more contexts.

Moreover, I believe usages in function returns will become more rare as 
projects move to C++14 and beyond (`auto` return type gives the same results in 
most interesting cases without code duplication).

Therefore, I'd not special-case for function return types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D72498#1813902 , @ilya-biryukov 
wrote:

> Could it be the case that we want to show the canonical types (i.e. without 
> all syntax sugar)?
>  Maybe we want both the normal type and the canonical type?


+1,I think it would be helpful to show the canonical type in this case.

  int main() {
  std::vector a;
  a.front(); // hover on front
  }

hover over the `front` , you'll see "instance-method `front` → 
`std::vector >::reference`".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

> what do you think about unwrapping decltype only when it's a return value 
> (optional: of a function whose leading return type is auto) to narrowly catch 
> this idiom?

I think it's worth fixing in the declarations too.

  int (int a, int b);
  
  template 
  auto call(Func &, Args &&... args)
  -> decltype(func(std::forward(args)...));
  
  template  void useRes(T &);
  
  void foo() {
// Under c++11 we don't have decltype(auto), using auto here will lose
// reference.
decltype(call(bar, 5, 6)) res = call(bar, 5, 6);
if (res) {
  // long code to process res
  useRes(res); // User wants to know the type of res here.
} else {
  // long code to process res
  useRes(res);
}
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D72498#1814286 , @lh123 wrote:

> In D72498#1814008 , @sammccall wrote:
>
> > I think i'm also comfortable with marking the linked bug as wontfix.
>
>
> The previous example is just minimal repo.
>  I think it's worth fixing in this case.
>
>   template  
>   auto sum(T1 , T2 ) ->decltype(t1 + t2))
>   {  
>   return t1 + t2;  
>   }
>


Thanks for the clearer example! I agree that's one where it'd be nice to show 
the result type and hovering over `decltype` won't give it to you.

But note that this patch doesn't handle the important use case of decltype as 
return type, really!
imagine `cout << sum('c', 4)`, and you want to know what type the function call 
has.
The natural thing is to hover over the function call, you'll see "function 
`sum` -> `decltype(t1 + t2)`" which is indeed pretty useless.
But this patch doesn't fix that behaviour, it only handles types of 
declarations.

@ilya-biryukov @kadircet what do you think about unwrapping decltype only when 
it's a return value (optional: of a function whose leading return type is auto) 
to narrowly catch this idiom?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[liu hui via Phabricator via cfe-commits]

lh123 added a comment.

In D72498#1814008 , @sammccall wrote:

> I think i'm also comfortable with marking the linked bug as wontfix.


The previous example is just minimal repo.

  template  
  auto sum(T1 , T2 ) ->decltype(t1 + t2))
  {  
return t1 + t2;  
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I think i'm also comfortable with marking the linked bug as wontfix.

It's a contrived example that makes clangd look silly (why decltype(a) instead 
of int?) but also the user look silly (why hover the variable rather than the 
decltype?).
Real examples are certainly more mixed.

The only thing that gives me pause is that it does seem to be decltype and auto 
belong in the same bucket (more so than typedef). If we can make those 
consistent in typeprinter, I'm happy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1813962 , @sammccall wrote:

> Maybe we want both the normal type and the canonical type?
>
> Canonical types are often *really* ugly, especially with STL types (we don't 
> have the "as written" form). And presenting the types twice might be at least 
> as confusing/noisy as helpful. But if you have examples where this would be 
> better, it'd be interesting.


I'm mostly trying to find a consistent rule we can apply to make these decision.
`auto`, `decltype` and typedefs are very similar in this regard, it's a bit 
confusing we use different rules for those. Although I can see how `auto` and 
`decltype` could easily be perceived differently and fall into a different 
group.

STL types are unfortunate, that is so true. And most other types aren't. It 
could be worth looking into the rules clang applies in diagnostics (have you 
seen those that say `type X(aka vector)`?
I have no easy answer, though, it feels this will inevitably lead to terrible 
presentations of some types. FWIW, showing underlying type of decltype could 
lead us there too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1813963 , @kadircet wrote:

> I think typedef and decltype have different nature, the latter is a lot more 
> obscure than the former, that was the reason why I handled decltypes 
> specifically.


I tend to disagree here. `decltype` is normally the last resort, so whatever it 
produces is probably super-obscure, would even expect it to be not 
representable in C++ in many cases.
E.g.

  auto Callback = []() { ... };
  decltype(Callback) ^a = Callback;

Typedefs are often used with simple types, so that's not necessarily the case.

  typedef unordered_map IntMap;
  IntMap ^a =;



> I agree with your suggestion for typedefs though, I think there would be 
> value in displaying the underlying type in hover card for type aliases to 
> reduce navigation.

Would definitely be helpful. If you feel we have some room in hover, I would 
love to have that. But there's a balance to be made, see Sam's comments about 
canonical types being obscure. I agree on 50% of the cases :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D72498#1813962 , @sammccall wrote:

> It's particularly unclear to me why typeprinter descends into auto but prints 
> decltype, but Kadir says that seems to be intentional.


Also don't see why they choose to have this inconsistency and I haven't seen 
any indication it's not a coincidence.
@kadircet, why do you think it's intentional? Should we put some comments there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as not done.
kadircet added a comment.

In D72498#1813899 , @ilya-biryukov 
wrote:

> This does not work for more complicated types, though?
>  E.g. `decltype(a+b)* a` or `vector a`?


yes, and I think we should have a more sophisticated way to print composite 
types including decltypes to cover more cases.

> Why do we prefer to drop `decltype()`, yet show the typedefs? Both could lead 
> to complicated types, arguably decltypes can be even worse than typedefs.
>  Could it be the case that we want to show the canonical types (i.e. without 
> all syntax sugar)?
>  Maybe we want both the normal type and the canonical type?

I think typedef and decltype have different nature, the latter is a lot more 
obscure than the former, that was the reason why I handled decltypes 
specifically. 
I agree with your suggestion for typedefs though, I think there would be value 
in displaying the underlying type in hover card for type aliases to reduce 
navigation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Yeah we discussed that case offline and I should have mentioned.
Resolving this in places other than the top-level would be nice and probably 
worthy of a comment at least. But this special case seems common enough to be 
worth having.
The only place fully resolving could reasonably be done I guess is TypePrinter, 
because we can't actually transform the types into the correct form IIUC.
It's particularly unclear to me why typeprinter descends into auto but prints 
decltype, but Kadir says that seems to be intentional.

> Could it be the case that we want to show the canonical types (i.e. without 
> all syntax sugar)?

Maybe we want both the normal type and the canonical type?

Canonical types are often *really* ugly, especially with STL types (we don't 
have the "as written" form). And presenting the types twice might be at least 
as confusing/noisy as helpful. But if you have examples where this would be 
better, it'd be interesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61742 tests passed, 0 failed 
and 780 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Could it be the case that we want to show the canonical types (i.e. without all 
syntax sugar)?
Maybe we want both the normal type and the canonical type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This does not work for more complicated types, though?
E.g. `decltype(a+b)* a` or `vector a`?

Why do we prefer to drop `decltype()`, yet show the typedefs? Both could lead 
to complicated types, arguably decltypes can be even worse than typedefs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 237284.
kadircet marked an inline comment as done.
kadircet added a comment.

- Update comment
- Iteratively resolve underlying decltypes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1501,6 +1501,40 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) c;
+  decltype(c) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(c) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  const decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "const decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -345,8 +345,14 @@
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
 fillFunctionTypeAndParams(HI, D, FD, Policy);
-  else if (const auto *VD = dyn_cast(D))
-HI.Type = VD->getType().getAsString(Policy);
+  else if (const auto *VD = dyn_cast(D)) {
+QualType QT = VD->getType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are 
going
+// to include spelling "decltype(X)" in `HoverInfo::Definition` anyway.
+while (auto *DT = QT->getAs())
+  QT = DT->getUnderlyingType();
+HI.Type = QT.getAsString(Policy);
+  }
 
   // Fill in value with evaluated initializer if possible.
   if (const auto *Var = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1501,6 +1501,40 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) c;
+  decltype(c) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(c) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  const decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "const decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -345,8 +345,14 @@
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
 fillFunctionTypeAndParams(HI, D, FD, Policy);
-  else if (const auto *VD = dyn_cast(D))
-HI.Type = VD->getType().getAsString(Policy);
+  else if (const auto *VD = dyn_cast(D)) {
+QualType QT = VD->getType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are going
+// to 

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61742 tests passed, 0 failed 
and 780 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/Hover.cpp:351
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are 
going
+// to include decltype(X) info in `HoverInfo::Definition` anyway.
+if (auto *DT = QT->getAs())

nit: going to include the spelling "decltype(X)" in ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Fixes https://github.com/clangd/clangd/issues/249


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72498

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1501,6 +1501,17 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -345,8 +345,14 @@
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
 fillFunctionTypeAndParams(HI, D, FD, Policy);
-  else if (const auto *VD = dyn_cast(D))
-HI.Type = VD->getType().getAsString(Policy);
+  else if (const auto *VD = dyn_cast(D)) {
+QualType QT = VD->getType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are 
going
+// to include decltype(X) info in `HoverInfo::Definition` anyway.
+if (auto *DT = QT->getAs())
+  QT = DT->getUnderlyingType();
+HI.Type = QT.getAsString(Policy);
+  }
 
   // Fill in value with evaluated initializer if possible.
   if (const auto *Var = dyn_cast(D)) {


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1501,6 +1501,17 @@
 HI.Name = "cls > >";
 HI.Documentation = "type of nested templates.";
   }},
+  {
+  R"cpp(// type with decltype
+  int a;
+  decltype(a) [[b^]] = a;)cpp",
+  [](HoverInfo ) {
+HI.Definition = "decltype(a) b = a";
+HI.Kind = index::SymbolKind::Variable;
+HI.NamespaceScope = "";
+HI.Name = "b";
+HI.Type = "int";
+  }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -345,8 +345,14 @@
   // Fill in types and params.
   if (const FunctionDecl *FD = getUnderlyingFunction(D))
 fillFunctionTypeAndParams(HI, D, FD, Policy);
-  else if (const auto *VD = dyn_cast(D))
-HI.Type = VD->getType().getAsString(Policy);
+  else if (const auto *VD = dyn_cast(D)) {
+QualType QT = VD->getType();
+// TypePrinter doesn't resolve decltypes, so resolve them here. We are going
+// to include decltype(X) info in `HoverInfo::Definition` anyway.
+if (auto *DT = QT->getAs())
+  QT = DT->getUnderlyingType();
+HI.Type = QT.getAsString(Policy);
+  }
 
   // Fill in value with evaluated initializer if possible.
   if (const auto *Var = dyn_cast(D)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits