DoDoENT 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;
----------------
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<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.
> 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 <line:20:1, col:21> col:21 bla 'NDArray<float, W>':'NDArray<float,
{{{0}}}>' callinit
`-CXXConstructExpr <col:21> 'NDArray<float, W>':'NDArray<float, {{{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 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.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
