dblaikie 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:
> 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<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.
> > 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.
>
> `PrintCanonicalTypes` is what controls output like whether we print a typedef
> name (not the canonical type) or the final underlying type all typedefs
> involved (the canonical type). It won't have an impact on things like whether
> we print the name of a structure or not.
>
> After looking into this, I think we do want changes here. I'm not 100%
> convinced we need a new policy member. I tried out printing the type names
> unconditionally and the results were a bit of a mixed bag (some diagnostics
> got more clear, other diagnostics didn't become more clear but also didn't
> become more confusing):
> ```
> error: 'note' diagnostics expected but not seen:
> File
> F:\source\llvm-project\clang\test\SemaTemplate\temp_arg_nontype_cxx20.cpp
> Line 189: in instantiation of template class 'Diags::X<{1, 2}>' requested here
> error: 'note' diagnostics seen but not expected:
> File
> F:\source\llvm-project\clang\test\SemaTemplate\temp_arg_nontype_cxx20.cpp
> Line 189: in instantiation of template class 'Diags::X<Diags::A{1, 2}>'
> requested here
> ```
> seems like a clarifying change, while:
> ```
> error: 'error' diagnostics expected but not seen:
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 19: no member named 'display' in 'ASCII<{"this nontype template argument is
> [...]"}>'
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 25: no member named 'display' in 'ASCII<{{119, 97, 105, 116, 32, 97, 32, 115,
> 27, 99, ...}}>'
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 33: no member named 'display' in 'ASCII<{"what??!"}>'
> error: 'error' diagnostics seen but not expected:
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 19: no member named 'display' in 'ASCII<Str<43>{"this nontype template
> argument is [...]"}>'
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 25: no member named 'display' in 'ASCII<Str<14>{{119, 97, 105, 116, 32, 97,
> 32, 115, 27, 99, ...}}>'
> File F:\source\llvm-project\clang\test\SemaCXX\cxx2a-nttp-printing.cpp Line
> 33: no member named 'display' in 'ASCII<Str<8>{"what??!"}>'
> ```
> seems like it's neither here nor there.
>
> @dblaikie, do you have feelings on how to go with this?
Yeah, I'm inclined to think that `Diags::X<{1, 2}>` is just too simplified -
it's unambiguous if the parameter isn't `auto`, but isn't valid syntax (so the
language still expects a type name there) & so maybe we should do the same in
diagnostics?
@aaron.ballman - though I'm still confused by the behavior-change when
`PrintCanonicalTypes = false` that causes the top level names to be printed?
Maybe that's just a weird case/red herring/bug in `PrintCanonicalTypes = true`
that skips those top level names and we could print them unconditionally?
@DoDoENT - I was/am curious if/why you need more explicitness than would be
provided by `PrintCanonicalTypes = false` - if the output was `NDArray<float,
Height{{{0}}}, Width{{{0}}}, Channels{{{0}}}>` would that be sufficient for
your needs? (I think in that case the name would be valid to use in code, which
I think is a reasonable basis to make the decision - but I'm not sure how to
justify adding all the intermediate names too)
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
