[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 468881.
DoDoENT retitled this revision from "Introduce the 
`AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy" to 
"Disambiguate type names when printing NTTP types".
DoDoENT edited the summary of this revision.
DoDoENT added a comment.

Implemented changes requested in the review:

- type name of NTTP is now always printed, as C++ requires
- keep full NTTP type printing behind a policy, for those that want/need that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,15 +115,65 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
 }
+
+TEST(TypePrinter, TemplateIdWithComplexFullTypeNTTP) {
+  constexpr char Code[] = R"cpp(
+  template< typename T, auto ... dims >
+  struct NDArray {};
+
+  struct Dimension
+  {
+  using value_type = unsigned short;
+
+  value_type size{ value_type( 0 ) };
+  };
+
+  template < typename ConcreteDim >
+  struct DimensionImpl : Dimension {};
+
+  struct Width: DimensionImpl< Width> {};
+  struct Height   : DimensionImpl< Height   > {};
+  struct Channels : DimensionImpl< Channels > {};
+
+  inline constexpr WidthW;
+  inline constexpr Height   H;
+  inline constexpr Channels C;
+
+  template< auto ... Dims >
+  consteval auto makeArray() noexcept
+  {
+  return NDArray< float, Dims ... >{};
+  }
+
+  [[ maybe_unused ]] auto x { makeArray< H, W, C >() };
+
+  )cpp";
+  auto Matcher = varDecl(
+  allOf(hasAttr(attr::Kind::Unused), hasType(qualType().bind("id";
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = false;
+  }));
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray{Dimension{0}}}, Width{DimensionImpl{Dimension{0}}}, Channels{DimensionImpl{Dimension{0}}}>)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = true;
+  }));
+}
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escap

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 468883.
DoDoENT added a comment.

Update comment/documentation of the new policy to correctly reflect the change 
in previous diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,15 +115,65 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
 }
+
+TEST(TypePrinter, TemplateIdWithComplexFullTypeNTTP) {
+  constexpr char Code[] = R"cpp(
+  template< typename T, auto ... dims >
+  struct NDArray {};
+
+  struct Dimension
+  {
+  using value_type = unsigned short;
+
+  value_type size{ value_type( 0 ) };
+  };
+
+  template < typename ConcreteDim >
+  struct DimensionImpl : Dimension {};
+
+  struct Width: DimensionImpl< Width> {};
+  struct Height   : DimensionImpl< Height   > {};
+  struct Channels : DimensionImpl< Channels > {};
+
+  inline constexpr WidthW;
+  inline constexpr Height   H;
+  inline constexpr Channels C;
+
+  template< auto ... Dims >
+  consteval auto makeArray() noexcept
+  {
+  return NDArray< float, Dims ... >{};
+  }
+
+  [[ maybe_unused ]] auto x { makeArray< H, W, C >() };
+
+  )cpp";
+  auto Matcher = varDecl(
+  allOf(hasAttr(attr::Kind::Unused), hasType(qualType().bind("id";
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = false;
+  }));
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray{Dimension{0}}}, Width{DimensionImpl{Dimension{0}}}, Channels{DimensionImpl{Dimension{0}}}>)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = true;
+  }));
+}
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHEC

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

In D134453#3868789 , @dblaikie wrote:

> I still don't think "keep full NTTP type printing behind a policy, for those 
> that want/need that" is a policy we should add. String printed names aren't 
> meant to be a tool for type reflection - the AST can be queried for that 
> information.

I agree on that matter.

However, I'm building my clang with this policy enabled by default to provide 
my developers with GCC/MSVC-like verbosity in diagnostic messages. They prefer 
it that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> (rabbit hole/tangent: Arguably what might be more useful to the user might be 
> a name that's not even usable in C++ - using the member variable names, as 
> well as the type names, though that'd be even more verbose... like 
> `t1` - which, I guess, if you had user defined types 
> for some kind of extra type safety, would get as verbose as 
> `t1` though I would expect 
> that would be the minority)

True, but if user defined type is defined as CRTP (very common for strong 
typedefs):

  template< typename T, typename U >
  struct StrongTypedef { T value; };
  
  struct Length : StrongTypedef< unsigned, Length > {};
  struct Width : StrongTypedef< unsigned, Width > {};
  
  struct Shape
  {
  Length length;
  Width  width;
  };
  
  template< Shape > struct S {};

you would get something like `S{.value = 50}}, .width = Width{StrongTypedef{.value = 70}}}>` (inspired by this GCC output 
), which is truly verbose. However, the 
current way of printing (assuming member names are printed) it would print 
something like `S<{.length = {{.value = 50}}, .width = {{.value = 70}}}>`. 
Ideally, in this scenario, probably the best possible output would be 
`S`. 
however, I'm not exactly sure how to achieve this (my patch would print 
`Shape`, but not `Length` and `Width` with my policy disabled.

Personally, I prefer full verbosity in all cases. Yes, it's a bit more to read, 
but does not confuse our less experienced c++ developers. This is why our 
internal build of clang has this enabled by default and I would be actually 
very happy if this patch gets accepted in a form where full types are always 
printed without the need for any new policies. If you are OK with that, I can 
update the patch. Just, please, let me know.




Comment at: clang/include/clang/AST/PrettyPrinter.h:78
 CleanUglifiedParameters(false), EntireContentsOfLargeArray(true),
-UseEnumerators(true) {}
+UseEnumerators(true), 
AlwaysIncludeTypeForNonTypeTemplateArgument(false) {}
 

aaron.ballman wrote:
> Should this be defaulting to false? I thought we wanted to always include the 
> type for NTTP printing?
I've set this to `false` by default to not interfere with the current behavior 
of the clang. 

However, after today's discussion, I think I can completely remove the policy 
and simply always print the full type. That would be option (1) from [[ 
https://reviews.llvm.org/D134453/new/#3869610 | this comment ]] and would make 
clang behave the same as GCC and MSVC.

If you are OK with that, I'll be happy to update the patch.



Comment at: clang/test/CodeGenCXX/debug-info-template.cpp:224
 template void f1();
-// CHECK: !DISubprogram(name: "f1", 
+// CHECK: !DISubprogram(name: "f1",
 // CHECK-SAME: templateParams: ![[RAW_FUNC_QUAL_ARGS:[0-9]*]],

dblaikie wrote:
> Looks like some unintended whitespace changes? (removing trailing whitespace 
> from these lines) 
Sorry about that. I've configured my editor to always remove trailing 
whitespace (we have this policy in the company) on save action. I can return 
the trailing whitespace if you want, although I would like to understand the 
reasoning for keeping the trailing whitespace...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> My thinking is that it's always more frustrating to have not enough 
> information in a diagnostic than too much information (both are problems in 
> their own ways though).
> ...
> I can understand it being less readable for deep nesting situations, but I do 
> not see why it would be prone to bad corner cases -- it's the most explicit 
> form we can write (and matches the behavior of all other C++ compilers, from 
> what we can tell).

I agree with @aaron.ballman. There is no ideal solution here, and I would too 
prefer to have too much information rather than not enough. GCC and MSVC are 
quite fine with printing full type names and it's up to IDE's and good editors 
and CI log parsers to parse that and present in a possibly better format.

If others would agree with that as well, I'll be happy to remove the policy 
from my PR completely and simply make clang print full explicit types always, 
just like GCC and MSVC do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 471081.
DoDoENT edited the summary of this revision.
DoDoENT added a comment.

Removed the policy and full explicit type printing behind it and keep only 
printing of first type name, as suggested by the review.

This is essentially (2) from here: https://reviews.llvm.org/D134453#3869610


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,14 +115,14 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abc">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abcd">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string"}>
   new ASCII<"print more characters as string">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string, no uplimit"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string, no uplimit"}>
   new ASCII<"print more characters as string, no uplimit">;
 }
 
 void wide() {
-  // CHECK{LITERAL}: ASCII<{L""}>
+  // CHECK{LITERAL}: ASCII{L""}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{L"the quick brown fox jumps"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{L"OVER THE LAZY DOG 0123456789"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 471083.
DoDoENT added a comment.

Restored trailing whitespace on untouched line that was automatically deleted 
by editor's save action.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,14 +115,14 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abc">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abcd">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string"}>
   new ASCII<"print more characters as string">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string, no uplimit"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string, no uplimit"}>
   new ASCII<"print more characters as string, no uplimit">;
 }
 
 void wide() {
-  // CHECK{LITERAL}: ASCII<{L""}>
+  // CHECK{LITERAL}: ASCII{L""}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{L"the quick brown fox jumps"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{L"OVER THE LAZY DOG 0123456789"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"escape\r\n"}>
+  // CHE

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 471145.
DoDoENT added a comment.

Updated clang release notes about improvement to diagnostics as requested by 
reviewers and added more vim-specific temp files to .gitignore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  .gitignore
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,14 +115,14 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abc">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abcd">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string"}>
   new ASCII<"print more characters as string">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string, no uplimit"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string, no uplimit"}>
   new ASCII<"print more characters as string, no uplimit">;
 }
 
 void wide() {
-  // CHECK{LITERAL}: ASCII<{L""}>
+  // CHECK{LITERAL}: ASCII{L""}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{L"the quick brown fox jumps"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{L"OVER THE LAZY DOG 0123456789"}>
   new ASCII;
-  // CHECK{LITERAL}: ASCII<{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{L"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII;
-  // CHECK{LITERAL}: ASCI

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> Do you need someone to land these changes on your behalf? If so, please let 
> me know what name and email address you'd like used for patch attribution.

Yes. My name is Nenad Mikša, e-mail address: dodoentertainm...@gmail.com




Comment at: .gitignore:23-26
+# other vim files (session, debugger, etc.)
+.vimrc
+.vimspector.json
+Session.vim

aaron.ballman wrote:
> I don't have any particular problems with this change, but it should be 
> separate from this patch (it's logically distinct from the other changes 
> here). I can handle fixing that up when landing the changes if we go that 
> route (I'll split it into two commits).
Should I put that in the separate PR then?

If you can handle this by splitting it into two commits, please do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 471174.
DoDoENT added a comment.

updated new tests that appeared after rebasing to main and removed vim-specific 
files from .gitignore (I'll keep that downstream).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/StmtPrinterTest.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,14 +115,14 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
Index: clang/unittests/AST/StmtPrinterTest.cpp
===
--- clang/unittests/AST/StmtPrinterTest.cpp
+++ clang/unittests/AST/StmtPrinterTest.cpp
@@ -174,7 +174,7 @@
 }
 )cpp",
 FunctionBodyMatcher("A"),
-"constexpr auto waldo = operator\"\"_c<{4}>();\n"));
+"constexpr auto waldo = operator\"\"_c();\n"));
 }
 
 TEST(StmtPrinter, TestCXXConversionDeclImplicit) {
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abc">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abcd">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string"}>
   new ASCII<"print more characters as string">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string, no uplimit"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string, no uplimit"}>
   new ASCII<"print more characters as string, no uplimit">;
 }
 
 void wide() {
-  // CHECK{LITERAL}: ASCII<{L""}>
+  // CHECK{LITERAL}: ASCII{L""}>
   new ASCII;
-  // CHECK{LITERAL}

[PATCH] D134453: Disambiguate type names when printing NTTP types

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT updated this revision to Diff 471204.
DoDoENT added a comment.

apply git-clang-format to changed files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/TemplateBase.cpp
  clang/test/CodeGenCXX/debug-info-template.cpp
  clang/test/SemaCXX/cxx2a-nttp-printing.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/test/SemaTemplate/temp_arg_string_printing.cpp
  clang/unittests/AST/StmtPrinterTest.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -115,14 +115,14 @@
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is [...]"}> &&)",
+  R"(ASCII{"this nontype template argument is [...]"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = false;
   }));
 
   ASSERT_TRUE(PrintedTypeMatches(
   Code, {"-std=c++20"}, Matcher,
-  R"(ASCII<{"this nontype template argument is too long to print"}> &&)",
+  R"(ASCII{"this nontype template argument is too long to print"}> &&)",
   [](PrintingPolicy &Policy) {
 Policy.EntireContentsOfLargeArray = true;
   }));
Index: clang/unittests/AST/StmtPrinterTest.cpp
===
--- clang/unittests/AST/StmtPrinterTest.cpp
+++ clang/unittests/AST/StmtPrinterTest.cpp
@@ -161,9 +161,9 @@
 }
 
 TEST(StmtPrinter, TestStringLiteralOperatorTemplate_Class) {
-  ASSERT_TRUE(
-  PrintedStmtCXXMatches(StdVer::CXX20,
-R"cpp(
+  ASSERT_TRUE(PrintedStmtCXXMatches(
+  StdVer::CXX20,
+  R"cpp(
 struct C {
   template  constexpr C(const char (&)[N]) : n(N) {}
   unsigned n;
@@ -173,8 +173,8 @@
   constexpr auto waldo = "abc"_c;
 }
 )cpp",
-FunctionBodyMatcher("A"),
-"constexpr auto waldo = operator\"\"_c<{4}>();\n"));
+  FunctionBodyMatcher("A"),
+  "constexpr auto waldo = operator\"\"_c();\n"));
 }
 
 TEST(StmtPrinter, TestCXXConversionDeclImplicit) {
Index: clang/test/SemaTemplate/temp_arg_string_printing.cpp
===
--- clang/test/SemaTemplate/temp_arg_string_printing.cpp
+++ clang/test/SemaTemplate/temp_arg_string_printing.cpp
@@ -19,111 +19,111 @@
 template  class ASCII {};
 
 void not_string() {
-  // CHECK{LITERAL}: ASCII<{{9, -1, 42}}>
+  // CHECK{LITERAL}: ASCII{{9, -1, 42}}>
   new ASCII<(int[]){9, -1, 42}>;
-  // CHECK{LITERAL}: ASCII<{{3.14e+00, 0.00e+00, 4.20e+01}}>
+  // CHECK{LITERAL}: ASCII{{3.14e+00, 0.00e+00, 4.20e+01}}>
   new ASCII<(double[]){3.14, 0., 42.}>;
 }
 
 void narrow() {
-  // CHECK{LITERAL}: ASCII<{""}>
+  // CHECK{LITERAL}: ASCII{""}>
   new ASCII<"">;
-  // CHECK{LITERAL}: ASCII<{"the quick brown fox jumps"}>
+  // CHECK{LITERAL}: ASCII{"the quick brown fox jumps"}>
   new ASCII<"the quick brown fox jumps">;
-  // CHECK{LITERAL}: ASCII<{"OVER THE LAZY DOG 0123456789"}>
+  // CHECK{LITERAL}: ASCII{"OVER THE LAZY DOG 0123456789"}>
   new ASCII<"OVER THE LAZY DOG 0123456789">;
-  // CHECK{LITERAL}: ASCII<{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
+  // CHECK{LITERAL}: ASCII{"\\`~!@#$%^&*()_+-={}[]|\'\";:,.<>?/"}>
   new ASCII?/)">;
-  // CHECK{LITERAL}: ASCII<{{101, 115, 99, 97, 112, 101, 0, 0}}>
+  // CHECK{LITERAL}: ASCII{{101, 115, 99, 97, 112, 101, 0, 0}}>
   new ASCII<"escape\0">;
-  // CHECK{LITERAL}: ASCII<{"escape\r\n"}>
+  // CHECK{LITERAL}: ASCII{"escape\r\n"}>
   new ASCII<"escape\r\n">;
-  // CHECK{LITERAL}: ASCII<{"escape\\\t\f\v"}>
+  // CHECK{LITERAL}: ASCII{"escape\\\t\f\v"}>
   new ASCII<"escape\\\t\f\v">;
-  // CHECK{LITERAL}: ASCII<{"escape\a\bc"}>
+  // CHECK{LITERAL}: ASCII{"escape\a\bc"}>
   new ASCII<"escape\a\b\c">;
-  // CHECK{LITERAL}: ASCII<{{110, 111, 116, 17, 0}}>
+  // CHECK{LITERAL}: ASCII{{110, 111, 116, 17, 0}}>
   new ASCII<"not\x11">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abc">;
-  // CHECK{LITERAL}: ASCII<{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
+  // CHECK{LITERAL}: ASCII{{18, 20, 127, 16, 1, 32, 97, 98, 99, 100, 0}}>
   new ASCII<"\x12\x14\x7f\x10\x01 abcd">;
-  // CHECK{LITERAL}: ASCII<{"print more characters as string"}>
+  // CHECK{LITERAL}: ASCII{"print more characters as string"}>
   new ASCII<"print more characters as string">;
-  // CHECK{LITERAL}: ASCII<{

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT created this revision.
Herald added a project: All.
DoDoENT requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If enabled, it will ensure that full type name is always printed.
This should fix issue reported here: 
https://github.com/llvm/llvm-project/issues/57562

Currently, I've disabled the policy by default, as enabling it makes some tests 
fail, notably:

- CodeGenCXX/debug-info-template.cpp
- SemaCXX/constexpr-printing.cpp
- SemaCXX/cxx2a-nttp-printing.cpp
- SemaTemplate/temp_arg_string_printing.cpp

However, my opinion is that those tests should be updated and new policy always 
enabled, to mimic GCC's behavior.

I've added 2 new tests in AST/TypePrinterTest.cpp, one of which actually 
demonstrates the issue I've had in the first place (which was also reported on 
GitHub).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134453

Files:
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -48,7 +48,7 @@
   std::string Code = R"cpp(
 namespace N {
   template  struct Type {};
-  
+
   template 
   void Foo(const Type &Param);
 }
@@ -127,3 +127,86 @@
 Policy.EntireContentsOfLargeArray = true;
   }));
 }
+
+TEST(TypePrinter, TemplateIdWithFullTypeNTTP) {
+  constexpr char Code[] = R"cpp(
+enum struct Encoding { UTF8, ASCII };
+template 
+struct Str {
+  constexpr Str(char const (&s)[N]) { __builtin_memcpy(value, s, N); }
+  char value[N];
+};
+template  class ASCII {};
+
+ASCII<"some string"> x;
+  )cpp";
+  auto Matcher = classTemplateSpecializationDecl(
+  hasName("ASCII"), has(cxxConstructorDecl(
+isMoveConstructor(),
+has(parmVarDecl(hasType(qualType().bind("id")));
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(ASCII{"some string"}> &&)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = true;
+  }));
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher, R"(ASCII<{"some string"}> &&)",
+  [](PrintingPolicy &Policy) {
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = false;
+  }));
+}
+
+TEST(TypePrinter, TemplateIdWithComplexFullTypeNTTP) {
+  constexpr char Code[] = R"cpp(
+  template< typename T, auto ... dims >
+  struct NDArray {};
+
+  struct Dimension
+  {
+  using value_type = unsigned short;
+
+  value_type size{ value_type( 0 ) };
+  };
+
+  template < typename ConcreteDim >
+  struct DimensionImpl : Dimension {};
+
+  struct Width: DimensionImpl< Width> {};
+  struct Height   : DimensionImpl< Height   > {};
+  struct Channels : DimensionImpl< Channels > {};
+
+  inline constexpr WidthW;
+  inline constexpr Height   H;
+  inline constexpr Channels C;
+
+  template< auto ... Dims >
+  consteval auto makeArray() noexcept
+  {
+  return NDArray< float, Dims ... >{};
+  }
+
+  [[ maybe_unused ]] auto x { makeArray< H, W, C >() };
+
+  )cpp";
+  auto Matcher = varDecl(
+  allOf(hasAttr(attr::Kind::Unused), hasType(qualType().bind("id";
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray)",
+  [](PrintingPolicy &Policy) {
+Policy.PrintCanonicalTypes = true;
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = false;
+  }));
+
+  ASSERT_TRUE(PrintedTypeMatches(
+  Code, {"-std=c++20"}, Matcher,
+  R"(NDArray{Dimension{0}}}, Width{DimensionImpl{Dimension{0}}}, Channels{DimensionImpl{Dimension{0}}}>)",
+  [](PrintingPolicy &Policy) {
+Policy.PrintCanonicalTypes = true;
+Policy.AlwaysIncludeTypeForNonTypeTemplateArgument = true;
+  }));
+}
Index: clang/lib/AST/TemplateBase.cpp
===
--- clang/lib/AST/TemplateBase.cpp
+++ clang/lib/AST/TemplateBase.cpp
@@ -432,10 +432,11 @@
   }
 
   case Declaration: {
-// FIXME: Include the type if it's not obvious from the context.
 NamedDecl *ND = getAsDecl();
 if (getParamTypeForDecl()->isRecordType()) {
   if (auto *TPO = dyn_cast(ND)) {
+if (Policy.AlwaysIncludeTypeForNonTypeTemplateArgument)
+  TPO->getType().getUnqualifiedType().print(Out, Policy);
 TPO->printAsInit(Out, Policy);
 break;
   }
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -892,6 +892,8 @@
 assert(BI != CD->bases_end());
 if (!First)
   Out << ", ";
+if (Policy.AlwaysIncludeT

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S" if enabled and as 
"S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;

dblaikie wrote:
> What does `PrintCanonicalTypes` have to do with this? Does it overlap with 
> this functionality in some way, but doesn't provide the functionality you 
> want in particular?
Thank you for the question. If you set the `PrintCanonicalTypes` to `false`, 
the `S` would be printed as `S` even without this 
patch. However, if you set it to `true`, it will be printed as `S<{1, 2}>`.

I don't fully understand why it does that, but it's quite annoying.

For a better example, please take a look at the 
`TemplateIdWithComplexFullTypeNTTP` unit tests that I've added: if 
`PrintCanonicalTypes` is set to `true`, the original print output of type is 
`NDArray`, and if set to `false` (which is 
default), the output is `NDArray` - so the NTTP type is neither fully written nor fully 
omitted, which is weird.

As I said, I don't really understand the idea behind `PrintCanonicalTypes`, but 
when my new `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, you will 
get the full type printed, regardless of value of `PrintCanonicalTypes` setting.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S" if enabled and as 
"S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;

aaron.ballman wrote:
> dblaikie wrote:
> > DoDoENT wrote:
> > > dblaikie wrote:
> > > > What does `PrintCanonicalTypes` have to do with this? Does it overlap 
> > > > with this functionality in some way, but doesn't provide the 
> > > > functionality you want in particular?
> > > Thank you for the question. If you set the `PrintCanonicalTypes` to 
> > > `false`, the `S` would be printed as `S` even 
> > > without this patch. However, if you set it to `true`, it will be printed 
> > > as `S<{1, 2}>`.
> > > 
> > > I don't fully understand why it does that, but it's quite annoying.
> > > 
> > > For a better example, please take a look at the 
> > > `TemplateIdWithComplexFullTypeNTTP` unit tests that I've added: if 
> > > `PrintCanonicalTypes` is set to `true`, the original print output of type 
> > > is `NDArray`, and if set to `false` 
> > > (which is default), the output is `NDArray > > Width{{{0}}}, Channels{{{0}}}>` - so the NTTP type is neither fully 
> > > written nor fully omitted, which is weird.
> > > 
> > > As I said, I don't really understand the idea behind 
> > > `PrintCanonicalTypes`, but when my new 
> > > `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, you will get 
> > > the full type printed, regardless of value of `PrintCanonicalTypes` 
> > > setting.
> > > 
> > Perhaps this might be more of a bug in PrintCanonicalTypes than something 
> > to add a separate flag for.
> > 
> > @aaron.ballman D2 for context here... 
> > 
> > Hmm, actually, just adding the top level `Height{{0}}, Width{{0}}, 
> > Channels{{0}}` is sufficient to make this code compile (whereas with the 
> > `{{{0}}}` it doesn't form a valid identifier.
> > 
> > So what's your use case for needing more explicitness than that top level? 
> > Perhaps this might be more of a bug in PrintCanonicalTypes than something 
> > to add a separate flag for.
> >
> > @aaron.ballman D2 for context here...
> 
> I looked over D2 again and haven't spotted anything with it that seems 
> amiss; the change there is to grab the canonical type before trying to print 
> it which is all the more I'd expect `PrintCanonicalTypes` to impact.
> 
> This looks like the behavior you'd get when you desugar the type. Check out 
> the AST dump for `s`: https://godbolt.org/z/vxh5j6qWr
> ```
> `-VarDecl  col:20 s 'S':'S<{1, 2}>' callinit
> ```
> We generate that type information at 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L663
>  for doing the AST dump, note how the second type printed is the desugared 
> type and that matches what we're seeing from the pretty printer.
> So what's your use case for needing more explicitness than that top level?

As I described in the [github 
issue](https://github.com/llvm/llvm-project/issues/57562), I'm trying to write 
a clang-based tool that will have different behavior if the printed `{{{0}}}` 
is actually `Width` than if its `Height` or anything else.

You can see the the issue in the AST dump for `bla`: 
https://godbolt.org/z/fMr4f13o3

The type is
```
`-VarDecl  col:21 bla 'NDArray':'NDArray' callinit
  `-CXXConstructExpr  'NDArray':'NDArray' 
'void () noexcept'
```

so it's unknown whether `{{{0}}}` represent the `Width` or `Height`. My patch 
makes it work exactly like GCC (see the comparison of error message between 
[clang 15 and GCC 12.1](https://godbolt.org/z/WenWe8caf).

> Perhaps this might be more of a bug in PrintCanonicalTypes than something to 
> add a separate flag for.

This was also my first thought and the first version of my patch (before even 
submitting it here) was to actually change the behavior of 
`PrintCanonicalTypes`. However, that change made some tests fail, as I 
described in the patch description:

- CodeGenCXX/debug-info-template.cpp
- SemaCXX/constexpr-printing.cpp
- SemaCXX/cxx2a-nttp-printing.cpp
- SemaTemplate/temp_arg_string_printing.cpp

Of course, it's possible to simply update the tests, but I actually don't fully 
understand what is the goal of `PrintCanonicalTypes` and whether its current 
behavior is actually desired or not, so I played it safe and introduced a new 
policy that is disabled by default until I get more feedback from more 
experienced LLVM developers.

The patch does solve my problem and since I'm building LLVM from source anyway, 
I can have it enabled in my fork.

I just want to see if it would also be beneficial to be introduced into the 
upstream LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

___
cfe-commits mailing list
cfe-commits@lists.llvm.or

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S" if enabled and as 
"S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;

dblaikie wrote:
> aaron.ballman wrote:
> > DoDoENT wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > DoDoENT wrote:
> > > > > > dblaikie wrote:
> > > > > > > What does `PrintCanonicalTypes` have to do with this? Does it 
> > > > > > > overlap with this functionality in some way, but doesn't provide 
> > > > > > > the functionality you want in particular?
> > > > > > Thank you for the question. If you set the `PrintCanonicalTypes` to 
> > > > > > `false`, the `S` would be printed as `S` 
> > > > > > even without this patch. However, if you set it to `true`, it will 
> > > > > > be printed as `S<{1, 2}>`.
> > > > > > 
> > > > > > I don't fully understand why it does that, but it's quite annoying.
> > > > > > 
> > > > > > For a better example, please take a look at the 
> > > > > > `TemplateIdWithComplexFullTypeNTTP` unit tests that I've added: if 
> > > > > > `PrintCanonicalTypes` is set to `true`, the original print output 
> > > > > > of type is `NDArray`, and if set 
> > > > > > to `false` (which is default), the output is `NDArray > > > > > Height{{{0}}}, Width{{{0}}}, Channels{{{0}}}>` - so the NTTP type 
> > > > > > is neither fully written nor fully omitted, which is weird.
> > > > > > 
> > > > > > As I said, I don't really understand the idea behind 
> > > > > > `PrintCanonicalTypes`, but when my new 
> > > > > > `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, you will 
> > > > > > get the full type printed, regardless of value of 
> > > > > > `PrintCanonicalTypes` setting.
> > > > > > 
> > > > > Perhaps this might be more of a bug in PrintCanonicalTypes than 
> > > > > something to add a separate flag for.
> > > > > 
> > > > > @aaron.ballman D2 for context here... 
> > > > > 
> > > > > Hmm, actually, just adding the top level `Height{{0}}, Width{{0}}, 
> > > > > Channels{{0}}` is sufficient to make this code compile (whereas with 
> > > > > the `{{{0}}}` it doesn't form a valid identifier.
> > > > > 
> > > > > So what's your use case for needing more explicitness than that top 
> > > > > level? 
> > > > > Perhaps this might be more of a bug in PrintCanonicalTypes than 
> > > > > something to add a separate flag for.
> > > > >
> > > > > @aaron.ballman D2 for context here...
> > > > 
> > > > I looked over D2 again and haven't spotted anything with it that 
> > > > seems amiss; the change there is to grab the canonical type before 
> > > > trying to print it which is all the more I'd expect 
> > > > `PrintCanonicalTypes` to impact.
> > > > 
> > > > This looks like the behavior you'd get when you desugar the type. Check 
> > > > out the AST dump for `s`: https://godbolt.org/z/vxh5j6qWr
> > > > ```
> > > > `-VarDecl  col:20 s 'S':'S<{1, 2}>' 
> > > > callinit
> > > > ```
> > > > We generate that type information at 
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L663
> > > >  for doing the AST dump, note how the second type printed is the 
> > > > desugared type and that matches what we're seeing from the pretty 
> > > > printer.
> > > > So what's your use case for needing more explicitness than that top 
> > > > level?
> > > 
> > > As I described in the [github 
> > > issue](https://github.com/llvm/llvm-project/issues/57562), I'm trying to 
> > > write a clang-based tool that will have different behavior if the printed 
> > > `{{{0}}}` is actually `Width` than if its `Height` or anything else.
> > > 
> > > You can see the the issue in the AST dump for `bla`: 
> > > https://godbolt.org/z/fMr4f13o3
> > > 
> > > The type is
> > > ```
> > > `-VarDecl  col:21 bla 'NDArray > > W>':'NDArray' callinit
> > >   `-CXXConstructExpr  'NDArray':'NDArray > > {{{0}}}>' 'void () noexcept'
> > > ```
> > > 
> > > so it's unknown whether `{{{0}}}` represent the `Width` or `Height`. My 
> > > patch makes it work exactly like GCC (see the comparison of error message 
> > > between [clang 15 and GCC 12.1](https://godbolt.org/z/WenWe8caf).
> > > 
> > > > Perhaps this might be more of a bug in PrintCanonicalTypes than 
> > > > something to add a separate flag for.
> > > 
> > > This was also my first thought and the first version of my patch (before 
> > > even submitting it here) was to actually change the behavior of 
> > > `PrintCanonicalTypes`. However, that change made some tests fail, as I 
> > > described in the patch description:
> > > 
> > > - CodeGenCXX/debug-info-template.cpp
> > > - SemaCXX/constexpr-printing.cpp
> > > - SemaCXX/cxx2a-nttp-printing.cpp
> > > - SemaTemplate/temp_arg_string_printing.cpp
> > > 
> > > Of course, it's possible to simply update the tests, but I actually don't 
> > > fully understand what is the

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S" if enabled and as 
"S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;

dblaikie wrote:
> DoDoENT wrote:
> > DoDoENT wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > DoDoENT wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > DoDoENT wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > What does `PrintCanonicalTypes` have to do with this? Does 
> > > > > > > > > > it overlap with this functionality in some way, but doesn't 
> > > > > > > > > > provide the functionality you want in particular?
> > > > > > > > > Thank you for the question. If you set the 
> > > > > > > > > `PrintCanonicalTypes` to `false`, the `S` would 
> > > > > > > > > be printed as `S` even without this patch. 
> > > > > > > > > However, if you set it to `true`, it will be printed as 
> > > > > > > > > `S<{1, 2}>`.
> > > > > > > > > 
> > > > > > > > > I don't fully understand why it does that, but it's quite 
> > > > > > > > > annoying.
> > > > > > > > > 
> > > > > > > > > For a better example, please take a look at the 
> > > > > > > > > `TemplateIdWithComplexFullTypeNTTP` unit tests that I've 
> > > > > > > > > added: if `PrintCanonicalTypes` is set to `true`, the 
> > > > > > > > > original print output of type is `NDArray > > > > > > > > {{{0}}}, {{{0}}}>`, and if set to `false` (which is default), 
> > > > > > > > > the output is `NDArray > > > > > > > > Channels{{{0}}}>` - so the NTTP type is neither fully written 
> > > > > > > > > nor fully omitted, which is weird.
> > > > > > > > > 
> > > > > > > > > As I said, I don't really understand the idea behind 
> > > > > > > > > `PrintCanonicalTypes`, but when my new 
> > > > > > > > > `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, you 
> > > > > > > > > will get the full type printed, regardless of value of 
> > > > > > > > > `PrintCanonicalTypes` setting.
> > > > > > > > > 
> > > > > > > > Perhaps this might be more of a bug in PrintCanonicalTypes than 
> > > > > > > > something to add a separate flag for.
> > > > > > > > 
> > > > > > > > @aaron.ballman D2 for context here... 
> > > > > > > > 
> > > > > > > > Hmm, actually, just adding the top level `Height{{0}}, 
> > > > > > > > Width{{0}}, Channels{{0}}` is sufficient to make this code 
> > > > > > > > compile (whereas with the `{{{0}}}` it doesn't form a valid 
> > > > > > > > identifier.
> > > > > > > > 
> > > > > > > > So what's your use case for needing more explicitness than that 
> > > > > > > > top level? 
> > > > > > > > Perhaps this might be more of a bug in PrintCanonicalTypes than 
> > > > > > > > something to add a separate flag for.
> > > > > > > >
> > > > > > > > @aaron.ballman D2 for context here...
> > > > > > > 
> > > > > > > I looked over D2 again and haven't spotted anything with it 
> > > > > > > that seems amiss; the change there is to grab the canonical type 
> > > > > > > before trying to print it which is all the more I'd expect 
> > > > > > > `PrintCanonicalTypes` to impact.
> > > > > > > 
> > > > > > > This looks like the behavior you'd get when you desugar the type. 
> > > > > > > Check out the AST dump for `s`: https://godbolt.org/z/vxh5j6qWr
> > > > > > > ```
> > > > > > > `-VarDecl  col:20 s 'S':'S<{1, 
> > > > > > > 2}>' callinit
> > > > > > > ```
> > > > > > > We generate that type information at 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L663
> > > > > > >  for doing the AST dump, note how the second type printed is the 
> > > > > > > desugared type and that matches what we're seeing from the pretty 
> > > > > > > printer.
> > > > > > > So what's your use case for needing more explicitness than that 
> > > > > > > top level?
> > > > > > 
> > > > > > As I described in the [github 
> > > > > > issue](https://github.com/llvm/llvm-project/issues/57562), I'm 
> > > > > > trying to write a clang-based tool that will have different 
> > > > > > behavior if the printed `{{{0}}}` is actually `Width` than if its 
> > > > > > `Height` or anything else.
> > > > > > 
> > > > > > You can see the the issue in the AST dump for `bla`: 
> > > > > > https://godbolt.org/z/fMr4f13o3
> > > > > > 
> > > > > > The type is
> > > > > > ```
> > > > > > `-VarDecl  col:21 bla 'NDArray > > > > > W>':'NDArray' callinit
> > > > > >   `-CXXConstructExpr  'NDArray':'NDArray > > > > > {{{0}}}>' 'void () noexcept'
> > > > > > ```
> > > > > > 
> > > > > > so it's unknown whether `{{{0}}}` represent the `Width` or 
> > > > > > `Height`. My patch makes it work exactly like GCC (see the 
> > > > > > comparison of error message between [clang 15 and GCC 
> > > > > > 12.1](https://godbolt.org/z/WenWe8caf).
> > > > > > 
> > > > > > > Perhaps this might be more of 

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S" if enabled and as 
"S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;

dblaikie wrote:
> DoDoENT wrote:
> > dblaikie wrote:
> > > DoDoENT wrote:
> > > > DoDoENT wrote:
> > > > > dblaikie wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > DoDoENT wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > DoDoENT wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > What does `PrintCanonicalTypes` have to do with this? 
> > > > > > > > > > > > Does it overlap with this functionality in some way, 
> > > > > > > > > > > > but doesn't provide the functionality you want in 
> > > > > > > > > > > > particular?
> > > > > > > > > > > Thank you for the question. If you set the 
> > > > > > > > > > > `PrintCanonicalTypes` to `false`, the `S` 
> > > > > > > > > > > would be printed as `S` even without this 
> > > > > > > > > > > patch. However, if you set it to `true`, it will be 
> > > > > > > > > > > printed as `S<{1, 2}>`.
> > > > > > > > > > > 
> > > > > > > > > > > I don't fully understand why it does that, but it's quite 
> > > > > > > > > > > annoying.
> > > > > > > > > > > 
> > > > > > > > > > > For a better example, please take a look at the 
> > > > > > > > > > > `TemplateIdWithComplexFullTypeNTTP` unit tests that I've 
> > > > > > > > > > > added: if `PrintCanonicalTypes` is set to `true`, the 
> > > > > > > > > > > original print output of type is `NDArray > > > > > > > > > > {{{0}}}, {{{0}}}>`, and if set to `false` (which is 
> > > > > > > > > > > default), the output is `NDArray > > > > > > > > > > Width{{{0}}}, Channels{{{0}}}>` - so the NTTP type is 
> > > > > > > > > > > neither fully written nor fully omitted, which is weird.
> > > > > > > > > > > 
> > > > > > > > > > > As I said, I don't really understand the idea behind 
> > > > > > > > > > > `PrintCanonicalTypes`, but when my new 
> > > > > > > > > > > `AlwaysIncludeTypeForNonTypeTemplateArgument` is enabled, 
> > > > > > > > > > > you will get the full type printed, regardless of value 
> > > > > > > > > > > of `PrintCanonicalTypes` setting.
> > > > > > > > > > > 
> > > > > > > > > > Perhaps this might be more of a bug in PrintCanonicalTypes 
> > > > > > > > > > than something to add a separate flag for.
> > > > > > > > > > 
> > > > > > > > > > @aaron.ballman D2 for context here... 
> > > > > > > > > > 
> > > > > > > > > > Hmm, actually, just adding the top level `Height{{0}}, 
> > > > > > > > > > Width{{0}}, Channels{{0}}` is sufficient to make this code 
> > > > > > > > > > compile (whereas with the `{{{0}}}` it doesn't form a valid 
> > > > > > > > > > identifier.
> > > > > > > > > > 
> > > > > > > > > > So what's your use case for needing more explicitness than 
> > > > > > > > > > that top level? 
> > > > > > > > > > Perhaps this might be more of a bug in PrintCanonicalTypes 
> > > > > > > > > > than something to add a separate flag for.
> > > > > > > > > >
> > > > > > > > > > @aaron.ballman D2 for context here...
> > > > > > > > > 
> > > > > > > > > I looked over D2 again and haven't spotted anything with 
> > > > > > > > > it that seems amiss; the change there is to grab the 
> > > > > > > > > canonical type before trying to print it which is all the 
> > > > > > > > > more I'd expect `PrintCanonicalTypes` to impact.
> > > > > > > > > 
> > > > > > > > > This looks like the behavior you'd get when you desugar the 
> > > > > > > > > type. Check out the AST dump for `s`: 
> > > > > > > > > https://godbolt.org/z/vxh5j6qWr
> > > > > > > > > ```
> > > > > > > > > `-VarDecl  col:20 s 'S':'S<{1, 
> > > > > > > > > 2}>' callinit
> > > > > > > > > ```
> > > > > > > > > We generate that type information at 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/TextNodeDumper.cpp#L663
> > > > > > > > >  for doing the AST dump, note how the second type printed is 
> > > > > > > > > the desugared type and that matches what we're seeing from 
> > > > > > > > > the pretty printer.
> > > > > > > > > So what's your use case for needing more explicitness than 
> > > > > > > > > that top level?
> > > > > > > > 
> > > > > > > > As I described in the [github 
> > > > > > > > issue](https://github.com/llvm/llvm-project/issues/57562), I'm 
> > > > > > > > trying to write a clang-based tool that will have different 
> > > > > > > > behavior if the printed `{{{0}}}` is actually `Width` than if 
> > > > > > > > its `Height` or anything else.
> > > > > > > > 
> > > > > > > > You can see the the issue in the AST dump for `bla`: 
> > > > > > > > https://godbolt.org/z/fMr4f13o3
> > > > > > > > 
> > > > > > > > The type is
> > > > > > > > ```
> > > > > > > > `-VarDecl  col:21 bla 'NDArray > > > > > > > W>':'NDArray' callinit
> > > 

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> Generally the way to do it, if you want to introspect into the type, its 
> template parameters, etc, then yes, keeping pointers to the AST or reparsing 
> the name with Clang as-needed, would be the way to go - or walking the AST 
> up-front and generating your own data structure (not necessarily a string) 
> with the data you want in some structured format.

This is precisely what I'm trying to avoid. Source files that include 
AST-related headers are extremely slow to compile, so I want to build a facade 
around the code that interacts with clang to keep my build times as low as 
possible. Thus, I will pay for the slow compilation only in a couple of files.

> Sorry, I'm confused - it sounded like you didn't actually want a string, 
> though - you want the type information to make various semantic-aware choices 
> in your tool, yes?

At the end of the line, I will need a string representation of the type name, 
at least in my current approach.

> Again, I'm not advocating for the printing as-is, I think adding the top 
> level name that disambiguates would be a good thing - and I think the GCC and 
> MSVC examples somewhat show why adding all the other layers would be harmful 
> to readability - there's a lot of text in those messages that doesn't 
> directly help the user and gets in the way of identifying the important 
> differences between the type names.

I think this is a matter of taste. In the example that you've shown, I 
personally prefer the verbosity of GCC and don't see it as "less readable", but 
as "more informative". However, I do understand that some people may prefer the 
output you suggested. What about making this configurable, i.e. behind some 
clang flag, so that developers that prefer verbosity can enable that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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

[PATCH] D134453: Introduce the `AlwaysIncludeTypeForNonTypeTemplateArgument` into printing policy

[Nenad Mikša via Phabricator via cfe-commits]

DoDoENT added a comment.

> Right - I'm trying to understand what your overall goals are and what clang 
> needs to facilitate them, if it's suitable to do so. And it seems to me that 
> the right way to support them, is for your approach to change - not to rely 
> on the type printing to communicate certain information, but to use the AST 
> to get the information you need (& if you want to encode that in a string, 
> that's up to you - I'd suggest encoding it in some more semantic data 
> structure (a struct, with variables, lists/vectors, etc, describing the 
> properties you want - so you don't have to rely on what clang does/doesn't 
> include in the text and have to parse that information out of the text later 
> on with your own home-grown C++ type parsing code, I guess)).

I've got a machine-generated c++ function in the form:

  c++
  template< typename Pixel >
  auto processImage( Image< Pixel > const & input )
  {
  auto result0{ processor.step1( input ) };
  auto result1{ processor.step2( result0 ) };
  ...
  auto resultN{ processor.stepNplus1( resultNminus1, resultK, resultM, ... 
) }; // for K, M, ... < N - 1
  return resultN;
  }

The `processor` is a graph processor that has steps that perform node 
computations (imagine it as neural network layers in machine learning 
inference).

Now, this works fine, but produces unnecessarily large binary, as types of most 
results don't depend on the `Pixel` template, but get instantiated anyway, thus 
making both binary larger and source slower to compile.

The idea of my tool is to use a clang to obtain concrete types of every 
`result0, result1, ..., resultN` and then decide whether its type depends on 
`Pixel` or not. Then use that information to cut the function into two 
functions - one that contains part of processing that depends on `Pixel` and 
the one that does not. The one that does not will be a normal function, while 
the one that does will remain in the function template and will call to the 
other after performing part of processing that depends on `Pixel`.

So, my idea is to obtain the types of each result as strings and then simply 
search for the keyword "Pixel"` in those strings to classify each result type. 
This approach works pretty well with my patch. Also, I find that much easier 
than navigating the AST.

But in general, you both are right - this tool could possibly be implemented 
differently and probably will in the future.

Now, let's keep our focus on verbosity in the diagnostic outputs, as this was 
the main reason I contributed this patch back to the upstream LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453

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