[PATCH] D93553: [clangd] Make our printing policies for Hover more consistent, especially tags

[Sam McCall via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb0615642f647: [clangd] Make our printing policies for Hover 
more consistent, especially tags (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93553

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
@@ -239,7 +239,7 @@
  HI.Name = "c";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "auto *c = ";
- HI.Type = "class (lambda) **";
+ HI.Type = "(lambda) **";
  HI.ReturnType = "bool";
  HI.Parameters = {
  {std::string("int"), std::string("T"), llvm::None},
@@ -611,7 +611,7 @@
   [](HoverInfo ) {
 HI.Name = "auto";
 HI.Kind = index::SymbolKind::TypeAlias;
-HI.Definition = "class Foo";
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -718,8 +718,8 @@
  HI.Definition = "X (float v)";
  HI.LocalScope = "X::";
  HI.Documentation = "Trivial setter for `Y`.";
- HI.Type = "struct X &(float)";
- HI.ReturnType = "struct X &";
+ HI.Type = "X &(float)";
+ HI.ReturnType = "X &";
  HI.Parameters.emplace();
  HI.Parameters->emplace_back();
  HI.Parameters->back().Type = "float";
@@ -1943,7 +1943,7 @@
 HI.Kind = index::SymbolKind::Variable;
 HI.NamespaceScope = "";
 HI.LocalScope = "test::";
-HI.Type = "struct Test &&";
+HI.Type = "Test &&";
 HI.Definition = "Test & = {}";
   }},
   {
@@ -2211,7 +2211,7 @@
   )cpp",
   [](HoverInfo ) {
 HI.Name = "this";
-HI.Definition = "Foo *";
+HI.Definition = "ns::Foo *";
   }},
   {
   R"cpp(// this expr for template class
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -49,16 +49,13 @@
 namespace clangd {
 namespace {
 
-PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
-  PrintingPolicy Policy(Base);
-
-  Policy.AnonymousTagLocations = false;
-  Policy.TerseOutput = true;
-  Policy.PolishForDeclaration = true;
-  Policy.ConstantsAsWritten = true;
-  Policy.SuppressTagKeyword = false;
-
-  return Policy;
+PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
+  Base.AnonymousTagLocations = false;
+  Base.TerseOutput = true;
+  Base.PolishForDeclaration = true;
+  Base.ConstantsAsWritten = true;
+  Base.SuppressTemplateArgsInCXXConstructors = true;
+  return Base;
 }
 
 /// Given a declaration \p D, return a human-readable string representing the
@@ -108,26 +105,33 @@
   return "";
 }
 
-std::string printDefinition(const Decl *D) {
+std::string printDefinition(const Decl *D, const PrintingPolicy ) {
   std::string Definition;
   llvm::raw_string_ostream OS(Definition);
-  PrintingPolicy Policy =
-  printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
-  Policy.IncludeTagDefinition = false;
-  Policy.SuppressTemplateArgsInCXXConstructors = true;
-  Policy.SuppressTagKeyword = true;
-  D->print(OS, Policy);
+  D->print(OS, PP);
   OS.flush();
   return Definition;
 }
 
-std::string printType(QualType QT, const PrintingPolicy ) {
+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 (!QT.isNull() && QT->isDecltypeType())
 QT = QT->getAs()->getUnderlyingType();
-  return QT.getAsString(Policy);
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  // Special case: if the outer type is a tag type without qualifiers, then
+  // include the tag for extra clarity.
+  // This isn't very idiomatic, so don't attempt it for complex cases, including
+  // pointers/references, template specializations, etc.
+  if (!QT.isNull() && !QT.hasQualifiers() && PP.SuppressTagKeyword) {
+if (auto *TT = llvm::dyn_cast(QT.getTypePtr()))
+  OS << TT->getDecl()->getKindName() << " ";
+  }
+  OS.flush();
+  QT.print(OS, PP);
+  return Result;
 }
 
 std::string printType(const TemplateTypeParmDecl *TTP) {
@@ -291,15 +295,15 @@
 }
 
 

[PATCH] D93553: [clangd] Make our printing policies for Hover more consistent, especially tags

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D93553#2463577 , @qchateau wrote:

> Looks good to me, it removes a lot of duplication and corner cases.
>
> I'd have printed the tag for reference and pointers as well (`class Foo*` 
> instead of `Foo*`). I don't think `Foo*` is much more understandable than 
> `Foo`, so we decide that printing `class Foo` is easier to understand for the 
> user, we should print `class Foo*` as well. 
> But I agree it's even less idiomatic, so I won't argue with your choice.

Yeah, `class Foo *` doesn't seem bad, though for some reason my brain struggles 
with `class Foo &&` :-)
I really think I like drawing the line *somewhere* in terms of complexity, but 
maybe it's not in the ideal place.

There's a pragmatic reason to draw it here: simply prepending "class " doesn't 
work in the presence of qualifiers. So the current patch will print `class Foo` 
but `const f(oo`, but at least the latter is rare. The inconsistency between 
`class Foo*` and `const Foo*` would be a bigger deal, I think. And getting 
`const class Foo*` without also producing `class Foo` would need 
changes to TypePrinter, which is a bit of a yak-shave.

So I think I'll land this as an improvement over the status quo, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93553

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

[PATCH] D93553: [clangd] Make our printing policies for Hover more consistent, especially tags

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added a comment.

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (`class Foo*` 
instead of `Foo*`). I don't think `Foo*` is much more understandable than 
`Foo`, so we decide that printing `class Foo` is easier to understand for the 
user, we should print `class Foo*` as well. 
But I agree it's even less idiomatic, so I won't argue with your choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93553

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

[PATCH] D93553: [clangd] Make our printing policies for Hover more consistent, especially tags

[Sam McCall via Phabricator via cfe-commits]

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

Different cases were using a bunch of different variants of the printing policy.
Each of these had something going for it, but the result was inconsistent.

Goals:

- single printing policy used (almost) everywhere
- avoid unidiomatic tags like `class vector`
- be informative and easy to understand

For tags, the solution I wound up with is: we print only the outer tag and only
in the simplest cases where this elaboration won't cause confusion.

For example:

- class X
- enum Foo
- vector
- X*

This seems to strike a nice balance of providing plenty of info/context in 
common
cases while never being confusing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93553

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
@@ -239,7 +239,7 @@
  HI.Name = "c";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "auto *c = ";
- HI.Type = "class (lambda) **";
+ HI.Type = "(lambda) **";
  HI.ReturnType = "bool";
  HI.Parameters = {
  {std::string("int"), std::string("T"), llvm::None},
@@ -611,7 +611,7 @@
   [](HoverInfo ) {
 HI.Name = "auto";
 HI.Kind = index::SymbolKind::TypeAlias;
-HI.Definition = "class Foo";
+HI.Definition = "class Foo";
   }},
   {// Falls back to primary template, when the type is not instantiated.
R"cpp(
@@ -718,8 +718,8 @@
  HI.Definition = "X (float v)";
  HI.LocalScope = "X::";
  HI.Documentation = "Trivial setter for `Y`.";
- HI.Type = "struct X &(float)";
- HI.ReturnType = "struct X &";
+ HI.Type = "X &(float)";
+ HI.ReturnType = "X &";
  HI.Parameters.emplace();
  HI.Parameters->emplace_back();
  HI.Parameters->back().Type = "float";
@@ -1943,7 +1943,7 @@
 HI.Kind = index::SymbolKind::Variable;
 HI.NamespaceScope = "";
 HI.LocalScope = "test::";
-HI.Type = "struct Test &&";
+HI.Type = "Test &&";
 HI.Definition = "Test & = {}";
   }},
   {
@@ -2211,7 +2211,7 @@
   )cpp",
   [](HoverInfo ) {
 HI.Name = "this";
-HI.Definition = "Foo *";
+HI.Definition = "ns::Foo *";
   }},
   {
   R"cpp(// this expr for template class
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -49,16 +49,13 @@
 namespace clangd {
 namespace {
 
-PrintingPolicy printingPolicyForDecls(PrintingPolicy Base) {
-  PrintingPolicy Policy(Base);
-
-  Policy.AnonymousTagLocations = false;
-  Policy.TerseOutput = true;
-  Policy.PolishForDeclaration = true;
-  Policy.ConstantsAsWritten = true;
-  Policy.SuppressTagKeyword = false;
-
-  return Policy;
+PrintingPolicy getPrintingPolicy(PrintingPolicy Base) {
+  Base.AnonymousTagLocations = false;
+  Base.TerseOutput = true;
+  Base.PolishForDeclaration = true;
+  Base.ConstantsAsWritten = true;
+  Base.SuppressTemplateArgsInCXXConstructors = true;
+  return Base;
 }
 
 /// Given a declaration \p D, return a human-readable string representing the
@@ -108,26 +105,33 @@
   return "";
 }
 
-std::string printDefinition(const Decl *D) {
+std::string printDefinition(const Decl *D, const PrintingPolicy ) {
   std::string Definition;
   llvm::raw_string_ostream OS(Definition);
-  PrintingPolicy Policy =
-  printingPolicyForDecls(D->getASTContext().getPrintingPolicy());
-  Policy.IncludeTagDefinition = false;
-  Policy.SuppressTemplateArgsInCXXConstructors = true;
-  Policy.SuppressTagKeyword = true;
-  D->print(OS, Policy);
+  D->print(OS, PP);
   OS.flush();
   return Definition;
 }
 
-std::string printType(QualType QT, const PrintingPolicy ) {
+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 (!QT.isNull() && QT->isDecltypeType())
 QT = QT->getAs()->getUnderlyingType();
-  return QT.getAsString(Policy);
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  // Special case: if the outer type is a tag type without qualifiers, then
+  // include the tag for extra clarity.
+  //