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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/PrettyPrinter.h:307
+  /// decltype(s) will be printed as "S<Point{1,2}>" if enabled and as "S<{1,2}>" if disabled,
+  /// regardless if PrintCanonicalTypes is enabled.
+  unsigned AlwaysIncludeTypeForNonTypeTemplateArgument : 1;
----------------
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<Point{1, 2}>` would be printed as `S<Point{1, 2}>` 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<float, {{{0}}}, {{{0}}}, {{{0}}}>`, and if set to `false` (which is default), the output is `NDArray<float, 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 D55552 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 D55552 for context here...

I looked over D55552 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 <line:3:1, col:20> col:20 s 'S<Point{1, 2}>':'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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134453