[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added a comment.

In D92041#2446366 , @sammccall wrote:

> Sorry for the delay here. Kadir is out on vacation.
>
> Yikes - it's a shame reusing our existing type printing doesn't do the right 
> thing, but injected-classname and partial specializations are indeed weird.
> I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather 
> than implement the workaround, but it's up to you.

Got it, thanks.
The weird string only happens in injected-classname and partial specialization, 
maybe we can add a PrintPolicy so that TypePrinter can handle this case? I have 
tried to extract template arguments by casting the injected-class to 
`ClassTemplatePartialSpecializationDecl`, and it seems work well.


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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 311256.
xndcn added a comment.

Thank you, it works like a charm!
For class withou template, `getHoverInfo(QualType ...)` will add namespace 
scope by default, so I have to add `SuppressScope` printpolicy here.


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

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,56 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+  // comment
+  namespace ns {
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "Foo *"; }},
+  {
+  R"cpp(// this expr for template class
+  namespace ns {
+template 
+class Foo {
+  Foo* bar() const {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "const Foo *"; }},
+  {
+  R"cpp(// this expr for specialization class
+  namespace ns {
+template  class Foo {};
+template <>
+struct Foo {
+  Foo* bar() {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "Foo *"; }},
+  {
+  R"cpp(// this expr for partial specialization struct
+  namespace ns {
+template  struct Foo {};
+template 
+struct Foo {
+  Foo* bar() const {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "const Foo *"; }},
   };
 
   // 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
@@ -552,7 +552,8 @@
 
 /// Generate a \p Hover object given the type \p T.
 HoverInfo getHoverContents(QualType T, ASTContext &ASTCtx,
-   const SymbolIndex *Index) {
+   const SymbolIndex *Index,
+   bool SuppressScope = false) {
   HoverInfo HI;
 
   if (const auto *D = T->getAsTagDecl()) {
@@ -566,6 +567,7 @@
 // Builtin types
 auto Policy = printingPolicyForDecls(ASTCtx.getPrintingPolicy());
 Policy.SuppressTagKeyword = true;
+Policy.SuppressScope = SuppressScope;
 HI.Name = T.getAsString(Policy);
   }
   return HI;
@@ -628,15 +630,29 @@
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
 return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with pointee type.
+  if (const CXXThisExpr *CTE = dyn_cast(E)) {
+QualType OriginThisType = CTE->getType()->getPointeeType();
+QualType ClassType = declaredType(OriginThisType->getAsTagDecl());
+// For partial specialization class, origin `this` pointee type will be
+// parsed as `InjectedClassNameType`, which will ouput template arguments
+// like "type-parameter-0-0". So we retrieve user written class type in this
+// case.
+QualType PrettyThisType = AST.getASTContext().getPointerType(
+QualType(ClassType.getTypePtr(), OriginThisType.getCVRQualifiers()));
+return getHoverContents(PrettyThisType, AST.getASTContext(), Index,
+/*SuppressScope=*/true);
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
@@ -861,7 +877,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added a comment.

In D92041#2454236 , @sammccall wrote:

> Very nice, thanks!
> I'll land this for you now.

Thank you very much!
Learned a lot about clangd and clang AST from this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn created this revision.
xndcn added reviewers: sammccall, kadircet.
xndcn added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
xndcn requested review of this revision.

How about add hover information for `this` expr?
It seems useful to show related information about the class for `this` expr 
sometimes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,23 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+   namespace ns {
+ class Foo {
+   Foo* bar() {
+ return [[t^his]];
+   }
+ };
+   };
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "this";
+HI.Type = "ns::Foo *";
+HI.Kind = index::SymbolKind::Unknown;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+  }},
   };
 
   // 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
@@ -606,6 +606,17 @@
   return HI;
 }
 
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
+  HoverInfo HI = getHoverContents(D, Index);
+  HI.Name = "this";
+  // TODO: determine the symbol kind.
+  HI.Kind = index::SymbolKind::Unknown;
+  HI.Type = printType(CTE->getType(), D->getASTContext().getPrintingPolicy());
+  return HI;
+}
+
 bool isLiteral(const Expr *E) {
   // Unfortunately there's no common base Literal classes inherits from
   // (apart from Expr), therefore these exclusions.
@@ -860,6 +871,8 @@
 if (!HI->Value)
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
+  } else if (const CXXThisExpr *CTE = N->ASTNode.get()) {
+HI = getHoverContents(CTE, Index);
   } else if (const Expr *E = N->ASTNode.get()) {
 HI = getHoverContents(E, AST);
   }


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2019,6 +2019,23 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+   namespace ns {
+ class Foo {
+   Foo* bar() {
+ return [[t^his]];
+   }
+ };
+   };
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "this";
+HI.Type = "ns::Foo *";
+HI.Kind = index::SymbolKind::Unknown;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+  }},
   };
 
   // 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
@@ -606,6 +606,17 @@
   return HI;
 }
 
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
+  HoverInfo HI = getHoverContents(D, Index);
+  HI.Name = "this";
+  // TODO: determine the symbol kind.
+  HI.Kind = index::SymbolKind::Unknown;
+  HI.Type = printType(CTE->getType(), D->getASTContext().getPrintingPolicy());
+  return HI;
+}
+
 bool isLiteral(const Expr *E) {
   // Unfortunately there's no common base Literal classes inherits from
   // (apart from Expr), therefore these exclusions.
@@ -860,6 +871,8 @@
 if (!HI->Value)
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
+  } else if (const CXXThisExpr *CTE = N->ASTNode.get()) {
+HI = getHoverContents(CTE, Index);
   } else if (const Expr *E = N->ASTNode.get()) {
 HI = getHoverContents(E, AST);
   }
___
cfe-commits mailing list
cfe-commi

[PATCH] D92041: Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:610
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();

kadircet wrote:
> what about using the existing `getHoverContents(QualType ..)` overload 
> instead ?
> what about using the existing `getHoverContents(QualType ..)` overload 
> instead ?





Comment at: clang-tools-extra/clangd/Hover.cpp:610
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();

xndcn wrote:
> kadircet wrote:
> > what about using the existing `getHoverContents(QualType ..)` overload 
> > instead ?
> > what about using the existing `getHoverContents(QualType ..)` overload 
> > instead ?
> 
> 
Thanks, I tried using `getHoverContents(QualType ..)` overload and the result 
looks more simplicity:
{F14312230}

The origin patch HoverInfo looks like:
{F14312245}

Both seems reasonable, not sure which one is better.



Comment at: clang-tools-extra/clangd/Hover.cpp:644
 // FIXME: Support hover for literals (esp user-defined)
 llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
   // There's not much value in hovering over "42" and getting a hover card

kadircet wrote:
> can you handle `CXXThisExpr` inside this function, instead of an extra branch 
> in the `getHover`?
Thanks, here need additional parameter `Index *`, will update patch soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:610
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();

kadircet wrote:
> xndcn wrote:
> > xndcn wrote:
> > > kadircet wrote:
> > > > what about using the existing `getHoverContents(QualType ..)` overload 
> > > > instead ?
> > > > what about using the existing `getHoverContents(QualType ..)` overload 
> > > > instead ?
> > > 
> > > 
> > Thanks, I tried using `getHoverContents(QualType ..)` overload and the 
> > result looks more simplicity:
> > {F14312230}
> > 
> > The origin patch HoverInfo looks like:
> > {F14312245}
> > 
> > Both seems reasonable, not sure which one is better.
> I am not sure if there's much value in repeating `this` and `type 
> definition`. So I would go with using the `QualType` overload here.
> But, rather than providing `CTE->getType()` directly, i believe we should 
> display information for the `PointeeType`, as it will also display comments 
> and such for `TagDecls`, rather than just providing the type-name.
Thanks! It looks more simplicity with `PointeeType`. Shall we keep the 
additional information like namespace and template parameters? So we can use 
`getHoverInfo(const NamedDecl ..)` overload?
{F14313888}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 307622.
xndcn added a comment.

Update the diff with `getHoverContents(const NamedDecl ..)` overload function.


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

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,22 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+   namespace ns {
+ class Foo {
+   Foo* bar() {
+ return [[t^his]];
+   }
+ };
+   };
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+  }},
   };
 
   // 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
@@ -630,7 +630,8 @@
 
 // Generates hover info for evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
@@ -647,6 +648,10 @@
 HI.Value = *Val;
 HI.Name = std::string(getNameForExpr(E));
 return HI;
+  } else if (const CXXThisExpr *CTE = dyn_cast(E)) {
+const NamedDecl *D = 
CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
+HI = getHoverContents(D, Index);
+return HI;
   }
   return llvm::None;
 }
@@ -861,7 +866,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other nodes?
   //  - built-in types


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2019,6 +2019,22 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+   namespace ns {
+ class Foo {
+   Foo* bar() {
+ return [[t^his]];
+   }
+ };
+   };
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+  }},
   };
 
   // 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
@@ -630,7 +630,8 @@
 
 // Generates hover info for evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
@@ -647,6 +648,10 @@
 HI.Value = *Val;
 HI.Name = std::string(getNameForExpr(E));
 return HI;
+  } else if (const CXXThisExpr *CTE = dyn_cast(E)) {
+const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
+HI = getHoverContents(D, Index);
+return HI;
   }
   return llvm::None;
 }
@@ -861,7 +866,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other nodes?
   //  - built-in types
___
cfe-commits mailing list
cfe-commits

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 307749.
xndcn added a comment.

Thanks! Update the commit as review comments.


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

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,61 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+  namespace ns {
+// comment
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+HI.Documentation = "comment";
+  }},
+  {
+  R"cpp(// this expr for template class
+  namespace ns {
+template 
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template  class Foo {}";
+HI.TemplateParameters = {
+{std::string("typename"), std::string("T"), llvm::None}};
+  }},
+  {
+  R"cpp(// this expr for specialization struct
+  namespace ns {
+template  struct Foo {};
+template <>
+struct Foo {
+  Foo* bar() {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Struct;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template <> struct Foo {}";
+  }},
   };
 
   // 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
@@ -628,15 +628,22 @@
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
 return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast(E)) {
+const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl();
+HI = getHoverContents(D, Index);
+return HI;
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
@@ -861,7 +868,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other nodes?
   //  - built-in types
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 307947.
xndcn added a comment.

Thanks. Update commit to fix the last nit.


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

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,61 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+  namespace ns {
+// comment
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "class Foo {}";
+HI.Documentation = "comment";
+  }},
+  {
+  R"cpp(// this expr for template class
+  namespace ns {
+template 
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Class;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template  class Foo {}";
+HI.TemplateParameters = {
+{std::string("typename"), std::string("T"), llvm::None}};
+  }},
+  {
+  R"cpp(// this expr for specialization struct
+  namespace ns {
+template  struct Foo {};
+template <>
+struct Foo {
+  Foo* bar() {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) {
+HI.Name = "Foo";
+HI.Kind = index::SymbolKind::Struct;
+HI.NamespaceScope = "ns::";
+HI.Definition = "template <> struct Foo {}";
+  }},
   };
 
   // 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
@@ -628,15 +628,21 @@
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
-llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
+llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST,
+   const SymbolIndex *Index) {
   // There's not much value in hovering over "42" and getting a hover card
   // saying "42 is an int", similar for other literals.
   if (isLiteral(E))
 return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast(E)) {
+const NamedDecl *D = CTE->getType()->getPointeeType()->getAsTagDecl();
+return getHoverContents(D, Index);
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
@@ -861,7 +867,7 @@
   HI->Value = printExprValue(N, AST.getASTContext());
 maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
   } else if (const Expr *E = N->ASTNode.get()) {
-HI = getHoverContents(E, AST);
+HI = getHoverContents(E, AST, Index);
   }
   // FIXME: support hovers for other nodes?
   //  - built-in types
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added a comment.

In D92041#2420251 , @kadircet wrote:

> Do you have commit access or should I commit this for you?

I still don't have commit access, so please help me commit this, thank you!


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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added a comment.

In D92041#2424869 , @kadircet wrote:

> can you give me an email address to associate the commit with?

xnd...@gmail.com, thank you.


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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn added a comment.

Thanks, it look more clear really. I'm trying to make the hover looks like 
`auto`


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

https://reviews.llvm.org/D92041

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

[PATCH] D92041: [clangd] Add hover info for `this` expr

[xndcn via Phabricator via cfe-commits]

xndcn updated this revision to Diff 308994.
xndcn added a comment.

`getHoverInfo(CXXThisExpr->getType()->getPointeeType(), ...)` does not output 
namespace scope and template parameters without specialization:
F14499632: Pointee.png 

while `getHoverInfo(CXXThisExpr->getType(), ...)` looks weird with partial 
specialization:
F14499662: QualType.png 

So I did some mix here...


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

https://reviews.llvm.org/D92041

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
@@ -2019,6 +2019,41 @@
 HI.NamespaceScope = "";
 HI.Definition = "@interface MYObject\n@end";
   }},
+  {
+  R"cpp(// this expr
+  // comment
+  class Foo {
+Foo* bar() {
+  return [[t^his]];
+}
+  };
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "Foo *"; }},
+  {
+  R"cpp(// this expr for template class
+  namespace ns {
+template 
+class Foo {
+  Foo* bar() {
+return [[t^his]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "ns::Foo *"; }},
+  {
+  R"cpp(// this expr for specialization struct
+  namespace ns {
+template  struct Foo {};
+template 
+struct Foo {
+  Foo* bar() {
+return [[thi^s]];
+  }
+};
+  }
+  )cpp",
+  [](HoverInfo &HI) { HI.Name = "ns::Foo *"; }},
   };
 
   // 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
@@ -628,7 +628,7 @@
   return llvm::StringLiteral("expression");
 }
 
-// Generates hover info for evaluatable expressions.
+// Generates hover info for `this` and evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
 llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST) {
   // There's not much value in hovering over "42" and getting a hover card
@@ -637,6 +637,41 @@
 return llvm::None;
 
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast(E)) {
+std::string Name;
+llvm::raw_string_ostream OS(Name);
+
+const NamedDecl *ND = CTE->getType()->getPointeeType()->getAsTagDecl();
+const auto NS = getNamespaceScope(ND);
+if (!NS.empty()) {
+  OS << NS << "::";
+}
+OS << printName(AST.getASTContext(), *ND);
+
+// printName(...) will not output template parameters without specialization
+// so fill in template params here
+if (const TemplateDecl *TD = ND->getDescribedTemplate()) {
+  OS << "<";
+  llvm::StringRef Sep = "";
+  const auto PP =
+  printingPolicyForDecls(AST.getASTContext().getPrintingPolicy());
+  for (const auto &P :
+   fetchTemplateParameters(TD->getTemplateParameters(), PP)) {
+OS << Sep;
+Sep = ", ";
+if (P.Name)
+  OS << P.Name;
+if (P.Default)
+  OS << " = " << P.Default;
+  }
+  OS << ">";
+}
+OS << " *";
+
+HI.Name = OS.str();
+return HI;
+  }
   // For expressions we currently print the type and the value, iff it is
   // evaluatable.
   if (auto Val = printExprValue(E, AST.getASTContext())) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits