[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<t2{.length=3, .width=7}>` - which, I guess, if you had user defined types for some kind of extra type safety, would get as verbose as `t1<Shape{Length{.length = 3}, Width{.width = 5}}>` 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<Shape{.length = Length{StrongTypedef<unsigned int, Length>{.value = 50}}, .width = Width{StrongTypedef<unsigned int, Width>{.value = 70}}}>` (inspired by this GCC output <https://godbolt.org/z/j1jsKrnhc>), 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<Shape{.length = Length{{.value = 50}}, .width = Width{{.value = 70}}}>`. 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<t1 () volatile, t1 () const volatile, t1 () &, t1 () &&>();
-// CHECK: !DISubprogram(name: "f1<RawFuncQual::t1 () volatile, RawFuncQual::t1 () const volatile, RawFuncQual::t1 () &, RawFuncQual::t1 () &&>", 
+// CHECK: !DISubprogram(name: "f1<RawFuncQual::t1 () volatile, RawFuncQual::t1 () const volatile, RawFuncQual::t1 () &, RawFuncQual::t1 () &&>",
 // 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