[PATCH] D77598: Integral template argument suffix and cast printing

[David Blaikie via Phabricator via cfe-commits]

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