[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry about the bad rebase, I landed the right version i think :-)

In D70325#1750496 , @sammccall wrote:

> In D70325#1749432 , @kadircet wrote:
>
> > LGTM, with a question. What about default template params? I believe we 
> > would also like to print them, could you add a test case for that?
>
>
> They're not printed, as the type is "as written". Added a testcase and a 
> FIXME.
>  (I don't think this case is terribly important - either behavior seems to 
> have its advantages, the combination of default parameters and partial 
> specialization is fairly rare, and not much confusion seems likely in 
> practice)


Forgot to mention, I didn't fix this because it's significantly more work: for 
Type you have access to the canonical args (with defaults, but with 
type-parameter-0-0 instead of T), and the as-written args (where defaults are 
omitted).
I think you have to trawl around special casing various Decls if you want this 
to work. I wanted to get the cheap improvement in because realistically I won't 
get to that refinement soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325



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

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe51484abd402: [clangd] Fix hover local scope to 
include class template params (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D70325?vs=229910=230004#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -910,13 +910,13 @@
}},
   // Constructor of partially-specialized class template
   {R"cpp(
-  template struct X;
+  template struct X;
   template struct X{ [[^X]](); };
   )cpp",
[](HoverInfo ) {
  HI.NamespaceScope = "";
  HI.Name = "X";
- HI.LocalScope = "X::";// FIXME: Should be X::
+ HI.LocalScope = "X::"; // FIXME: X::
  HI.Kind = SymbolKind::Constructor;
  HI.ReturnType = "X";
  HI.Definition = "X()";
@@ -1029,8 +1029,8 @@
  HI.Type = "enum Color";
  HI.Value = "1";
}},
-  // FIXME: We should use the Decl referenced, even if it comes from an
-  // implicit instantiation.
+  // FIXME: We should use the Decl referenced, even if from an implicit
+  // instantiation. Then the scope would be Add<1, 2> and the value 3.
   {R"cpp(
 template struct Add {
   static constexpr int result = a + b;
@@ -1043,7 +1043,7 @@
  HI.Kind = SymbolKind::Property;
  HI.Type = "const int";
  HI.NamespaceScope = "";
- HI.LocalScope = "Add::";
+ HI.LocalScope = "Add::";
}},
   {R"cpp(
 const char *[[ba^r]] = "1234";
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -414,12 +414,12 @@
 static std::string getLocalScope(const Decl *D) {
   std::vector Scopes;
   const DeclContext *DC = D->getDeclContext();
-  auto GetName = [](const Decl *D) {
-const NamedDecl *ND = dyn_cast(D);
-std::string Name = ND->getNameAsString();
-// FIXME(sammccall): include template params/specialization args?.
-if (!Name.empty())
-  return Name;
+  auto GetName = [](const TypeDecl *D) {
+if (!D->getDeclName().isEmpty()) {
+  PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
+  Policy.SuppressScope = true;
+  return declaredType(D).getAsString(Policy);
+}
 if (auto RD = dyn_cast(D))
   return ("(anonymous " + RD->getKindName() + ")").str();
 return std::string("");


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -910,13 +910,13 @@
}},
   // Constructor of partially-specialized class template
   {R"cpp(
-  template struct X;
+  template struct X;
   template struct X{ [[^X]](); };
   )cpp",
[](HoverInfo ) {
  HI.NamespaceScope = "";
  HI.Name = "X";
- HI.LocalScope = "X::";// FIXME: Should be X::
+ HI.LocalScope = "X::"; // FIXME: X::
  HI.Kind = SymbolKind::Constructor;
  HI.ReturnType = "X";
  HI.Definition = "X()";
@@ -1029,8 +1029,8 @@
  HI.Type = "enum Color";
  HI.Value = "1";
}},
-  // FIXME: We should use the Decl referenced, even if it comes from an
-  // implicit instantiation.
+  // FIXME: We should use the Decl referenced, even if from an implicit
+  // instantiation. Then the scope would be Add<1, 2> and the value 3.
   {R"cpp(
 template struct Add {
   static constexpr int result = a + b;
@@ -1043,7 +1043,7 @@
  HI.Kind = SymbolKind::Property;
  HI.Type = "const int";
  HI.NamespaceScope = "";
- HI.LocalScope = "Add::";
+ HI.LocalScope = "Add::";
}},
   {R"cpp(
 const char *[[ba^r]] = "1234";
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -414,12 +414,12 @@
 static std::string getLocalScope(const Decl *D) {
   std::vector Scopes;
   const DeclContext *DC = D->getDeclContext();
-  auto GetName = [](const Decl *D) {
-const NamedDecl *ND = dyn_cast(D);
-std::string Name = ND->getNameAsString();
-// FIXME(sammccall): include template params/specialization args?.
-if (!Name.empty())
-  

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks! but again it looks as if rebasing went wrong :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325



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

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D70325#1749432 , @kadircet wrote:

> LGTM, with a question. What about default template params? I believe we would 
> also like to print them, could you add a test case for that?


They're not printed, as the type is "as written". Added a testcase and a FIXME.
(I don't think this case is terribly important - either behavior seems to have 
its advantages, the combination of default parameters and partial 
specialization is fairly rare, and not much confusion seems likely in practice)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325



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

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 229910.
sammccall added a comment.

add fixme


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -910,16 +910,15 @@
}},
   // Constructor of partially-specialized class template
   {R"cpp(
-  template struct X;
+  template struct X;
   template struct X{ [[^X]](); };
   )cpp",
[](HoverInfo ) {
  HI.NamespaceScope = "";
  HI.Name = "X";
- HI.LocalScope = "X::";// FIXME: Should be X::
+ HI.LocalScope = "X::"; // FIXME: X::
  HI.Kind = SymbolKind::Constructor;
- HI.Type = "void ()";  // FIXME: Should be None
- HI.ReturnType = "void";   // FIXME: Should be None or X
+ HI.ReturnType = "X";
  HI.Definition = "X()";
  HI.Parameters.emplace();
}},
@@ -1020,8 +1019,8 @@
  HI.Type = "enum Color";
  HI.Value = "1";
}},
-  // FIXME: We should use the Decl referenced, even if it comes from an
-  // implicit instantiation.
+  // FIXME: We should use the Decl referenced, even if from an implicit
+  // instantiation. Then the scope would be Add<1, 2> and the value 3.
   {R"cpp(
 template struct Add {
   static constexpr int result = a + b;
@@ -1034,7 +1033,7 @@
  HI.Kind = SymbolKind::Property;
  HI.Type = "const int";
  HI.NamespaceScope = "";
- HI.LocalScope = "Add::";
+ HI.LocalScope = "Add::";
}},
   {R"cpp(
 const char *[[ba^r]] = "1234";
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -414,12 +414,12 @@
 static std::string getLocalScope(const Decl *D) {
   std::vector Scopes;
   const DeclContext *DC = D->getDeclContext();
-  auto GetName = [](const Decl *D) {
-const NamedDecl *ND = dyn_cast(D);
-std::string Name = ND->getNameAsString();
-// FIXME(sammccall): include template params/specialization args?.
-if (!Name.empty())
-  return Name;
+  auto GetName = [](const TypeDecl *D) {
+if (!D->getDeclName().isEmpty()) {
+  PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
+  Policy.SuppressScope = true;
+  return declaredType(D).getAsString(Policy);
+}
 if (auto RD = dyn_cast(D))
   return ("(anonymous " + RD->getKindName() + ")").str();
 return std::string("");
@@ -566,6 +566,51 @@
   Req, [&](const Symbol ) { Hover.Documentation = S.Documentation; });
 }
 
+// Populates Type, ReturnType, and Parameters for function-like decls.
+static void fillFunctionTypeAndParams(HoverInfo , const Decl *D,
+  const FunctionDecl *FD,
+  const PrintingPolicy ) {
+  HI.Parameters.emplace();
+  for (const ParmVarDecl *PVD : FD->parameters()) {
+HI.Parameters->emplace_back();
+auto  = HI.Parameters->back();
+if (!PVD->getType().isNull()) {
+  P.Type.emplace();
+  llvm::raw_string_ostream OS(*P.Type);
+  PVD->getType().print(OS, Policy);
+} else {
+  std::string Param;
+  llvm::raw_string_ostream OS(Param);
+  PVD->dump(OS);
+  OS.flush();
+  elog("Got param with null type: {0}", Param);
+}
+if (!PVD->getName().empty())
+  P.Name = PVD->getNameAsString();
+if (PVD->hasDefaultArg()) {
+  P.Default.emplace();
+  llvm::raw_string_ostream Out(*P.Default);
+  PVD->getDefaultArg()->printPretty(Out, nullptr, Policy);
+}
+  }
+
+  if (const auto* CCD = llvm::dyn_cast(FD)) {
+// Constructor's "return type" is the class type.
+HI.ReturnType = declaredType(CCD->getParent()).getAsString(Policy);
+// Don't provide any type for the constructor itself.
+  } else if (const auto* CDD = llvm::dyn_cast(FD)){
+HI.ReturnType = "void";
+  } else {
+HI.ReturnType = FD->getReturnType().getAsString(Policy);
+
+QualType FunctionType = FD->getType();
+if (const VarDecl *VD = llvm::dyn_cast(D)) // Lambdas
+  FunctionType = VD->getType().getDesugaredType(D->getASTContext());
+HI.Type = FunctionType.getAsString(Policy);
+  }
+  // FIXME: handle variadics.
+}
+
 /// Generate a \p Hover object given the declaration \p D.
 static HoverInfo getHoverContents(const Decl *D, const SymbolIndex *Index) {
   HoverInfo HI;
@@ -601,45 +646,7 @@
 
   // Fill in types and params.
   if 

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

LGTM, with a question. What about default template params? I believe we would 
also like to print them, could you add a test case for that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325



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

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

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

merge_guards_bot added a comment.

Build result: fail - 60138 tests passed, 2 failed and 729 were skipped.

  failed: LLVM.CodeGen/NVPTX/vectorize-misaligned.ll
  failed: LLVM.Transforms/LoadStoreVectorizer/AMDGPU/merge-stores.ll

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70325



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

[PATCH] D70325: [clangd] Fix hover 'local scope' to include class template params

[Sam McCall via Phabricator via cfe-commits]

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

Fixes the last part of https://github.com/clangd/clangd/issues/76


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70325

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -916,7 +916,7 @@
[](HoverInfo ) {
  HI.NamespaceScope = "";
  HI.Name = "X";
- HI.LocalScope = "X::";// FIXME: Should be X::
+ HI.LocalScope = "X::";
  HI.Kind = SymbolKind::Constructor;
  HI.ReturnType = "X";
  HI.Definition = "X()";
@@ -1019,8 +1019,8 @@
  HI.Type = "enum Color";
  HI.Value = "1";
}},
-  // FIXME: We should use the Decl referenced, even if it comes from an
-  // implicit instantiation.
+  // FIXME: We should use the Decl referenced, even if from an implicit
+  // instantiation. Then the scope would be Add<1, 2> and the value 3.
   {R"cpp(
 template struct Add {
   static constexpr int result = a + b;
@@ -1033,7 +1033,7 @@
  HI.Kind = SymbolKind::Property;
  HI.Type = "const int";
  HI.NamespaceScope = "";
- HI.LocalScope = "Add::";
+ HI.LocalScope = "Add::";
}},
   {R"cpp(
 const char *[[ba^r]] = "1234";
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -414,12 +414,12 @@
 static std::string getLocalScope(const Decl *D) {
   std::vector Scopes;
   const DeclContext *DC = D->getDeclContext();
-  auto GetName = [](const Decl *D) {
-const NamedDecl *ND = dyn_cast(D);
-std::string Name = ND->getNameAsString();
-// FIXME(sammccall): include template params/specialization args?.
-if (!Name.empty())
-  return Name;
+  auto GetName = [](const TypeDecl *D) {
+if (!D->getDeclName().isEmpty()) {
+  PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
+  Policy.SuppressScope = true;
+  return declaredType(D).getAsString(Policy);
+}
 if (auto RD = dyn_cast(D))
   return ("(anonymous " + RD->getKindName() + ")").str();
 return std::string("");


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -916,7 +916,7 @@
[](HoverInfo ) {
  HI.NamespaceScope = "";
  HI.Name = "X";
- HI.LocalScope = "X::";// FIXME: Should be X::
+ HI.LocalScope = "X::";
  HI.Kind = SymbolKind::Constructor;
  HI.ReturnType = "X";
  HI.Definition = "X()";
@@ -1019,8 +1019,8 @@
  HI.Type = "enum Color";
  HI.Value = "1";
}},
-  // FIXME: We should use the Decl referenced, even if it comes from an
-  // implicit instantiation.
+  // FIXME: We should use the Decl referenced, even if from an implicit
+  // instantiation. Then the scope would be Add<1, 2> and the value 3.
   {R"cpp(
 template struct Add {
   static constexpr int result = a + b;
@@ -1033,7 +1033,7 @@
  HI.Kind = SymbolKind::Property;
  HI.Type = "const int";
  HI.NamespaceScope = "";
- HI.LocalScope = "Add::";
+ HI.LocalScope = "Add::";
}},
   {R"cpp(
 const char *[[ba^r]] = "1234";
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -414,12 +414,12 @@
 static std::string getLocalScope(const Decl *D) {
   std::vector Scopes;
   const DeclContext *DC = D->getDeclContext();
-  auto GetName = [](const Decl *D) {
-const NamedDecl *ND = dyn_cast(D);
-std::string Name = ND->getNameAsString();
-// FIXME(sammccall): include template params/specialization args?.
-if (!Name.empty())
-  return Name;
+  auto GetName = [](const TypeDecl *D) {
+if (!D->getDeclName().isEmpty()) {
+  PrintingPolicy Policy = D->getASTContext().getPrintingPolicy();
+  Policy.SuppressScope = true;
+  return declaredType(D).getAsString(Policy);
+}
 if (auto RD = dyn_cast(D))
   return ("(anonymous " + RD->getKindName() + ")").str();
 return std::string("");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org