dblaikie added inline comments.
================
Comment at: clang/lib/AST/DeclPrinter.cpp:1101
Out << ", ";
- Args[I].print(Policy, Out);
+ if (TemplOverloaded || !Params)
+ Args[I].print(Policy, Out, /*IncludeType*/ true);
----------------
rsmith wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Looks like this (& the `TemplOverloaded` in the other function, below) is
> > > undertested.
> > >
> > > Hardcoding:
> > > true in this function results in no failures
> > > false in this function results in 1 failure in
> > > `SemaTemplate/temp_arg_enum_printing.cpp`
> > > - this failure, at least to me, seems to not have much to do with
> > > overloading - at least syntactically foo<2> (where the parameter is an
> > > enum type) doesn't compile, not due to any overloading ambiguity, but due
> > > to a lack of implicit conversion from int to the enum type, I think? and
> > > the example doesn't look like it involves any overloading...
> > >
> > > true in the other overload results in no failures
> > > false in the other overload results in no failures
> > >
> > > I came across this because I was confused by how this feature works -
> > > when the suffixes are used and when they are not to better understand the
> > > implications this might have for debug info. (though I'm still generally
> > > leaning towards just always putting the suffixes on for debug info)
> > >
> > > @rsmith - could you give an example of what you meant by passing in a
> > > parameter when the template is overloaded & that should use the suffix?
> > > My simple examples, like this:
> > > ```
> > > template<unsigned V>
> > > void f1() { }
> > > template<long L>
> > > void f1() { }
> > > int main() {
> > > f1<3U>();
> > > }
> > > ```
> > > That's just ambiguous, apparently, and doesn't compile despite the type
> > > suffix on the literal. If I put a parameter on one of the functions it
> > > doesn't seem to trigger the "TemplOverloaded" case - both instantiations
> > > still get un-suffixed names "f1<3>"...
> > Ping on this (posted https://reviews.llvm.org/D111477 for some further
> > discussion on the debug info side of things)
> I think the `TemplOverloaded` parameter is unnecessary: passing a null
> `Params` list will result in `shouldIncludeTypeForArgument` returning `true`,
> which would have the same effect as passing in `TemplOverloaded == true`. We
> don't need two different ways to request that fully-unambiguous printing is
> used for every argument, so removing this flag and passing a null `Params`
> list instead might be a good idea.
>
> Here's an unambiguous example where we need suffixes to identify which
> specialization we're referring to:
> ```
> template<long> void f() {}
> void (*p)() = f<0>;
> template<unsigned = 0> void f() {}
> void (*q)() = f<>;
> ```
>
> For this, `-ast-print` prints:
> ```
> template <long> void f() {
> }
> template<> void f<0L>() {
> }
> void (*p)() = f<0>;
> template <unsigned int = 0> void f() {
> }
> template<> void f<0U>() {
> }
> void (*q)() = f<>;
> ```
> ... where the `0U` is intended to indicate that the second specialization is
> for the second template not the first. (This `-ast-print` output isn't
> actually valid code because `f<0U>` is still ambiguous, but we've done the
> best we can.)
Thanks for the details!
Removed `TemplOverloaded` here: 5de369056dee2c4de81625cb05a5c212a0bdc053
(notes: The `DeclPrinter::VisitCXXRecordDecl` support for including/omiting
types is untested (making it always print suffixes didn't fail any tests) - I
was confused why that was, expecting some tests to fail even incidentally - but
seems that is only used for ast-print? Whereas printing of
`ClassTemplateSpecializationDecl` for diagnostics uses `getNameForDiagnostic`
which I guess is fair - though could these share more of their implementation?
Looks like they currently don't share template argument printing, the ast-print
uses `DeclPrinter::printTemplateArguments` and the diagnostic stuff uses
`TypePrinter.cpp:printTo` (so I guess function templates have another/third way
of printing template arguments?)
Did eventually find at least a way to test the
`DeclPrinter::VisitCXXRecordDecl` case, committed
in400eb59adf43b29af3117c163cf770e6d6e514f7 (& found some minor ast-print bugs
to commit as follow-ups in 604446aa6b41461e2691c9f4253e9ef70a5d68e4 and
b2589e326ba4407d8314938a4f7498086e2203ea) - open to ideas/suggestions if
there's other testing that might be suitable as well or instead.
Added your (@rsmith's) test as well in 50fdd7df827137c8465abafa82a6bae7c87096c5.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77598/new/
https://reviews.llvm.org/D77598
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
