Integral template argument suffix and cast printing

[reikdas]

Summary

Added suffixes and casts, where relevant, to integral types.

[rsmith]

clang/include/clang/AST/DeclTemplate.h

213

We should return true in this case; we don't know what the corresponding parameter is so we don't know if the type matters.

clang/lib/AST/ASTContext.cpp

7462 ↗

(On Diff #307587)

Pass nullptr here; for ObjC @encode we presumably want fully-explicit output always.

clang/lib/AST/DeclPrinter.cpp

654–655

We should conservatively assume that it is, rather than potentially printing out an ambiguous AST fragment.

1000–1003

This is a class template specialization, not a function template specialization. It can't be overloaded.

clang/lib/AST/StmtPrinter.cpp

1417

This isn't right: getDescribedTemplateParams handles the case where the declaration is the pattern in a template definition, not where it's a template instantiation. What you want here is:

if (auto *FTD = FD->getPrimaryTemplate())
  TPL = FTD->getTemplateParameters();

You should also take into account Node->hadMultipleCandidates() here.

Testcase for this would be something like:

struct A {
  template<long, auto> void f();
};
void g(A a) { a.f<0L, 0L>(); }

... for which -ast-print should produce f<0, 0L>.

1420

This is likewise not right. In this case we always know there's a template, so it's slightly simpler:

TPL = TD->getSpecializedTemplate()->getTemplateParameters();

Please also rename TD to something more appropriate.

Testcase for this would be something like:

struct A {
  template<long> static int x;
  template<auto> static int y;
};
int k = A().x<0L> + A().y<0L>;

... for which -ast-print should produce x<0> but y<0L>.

clang/lib/AST/TemplateBase.cpp

54

I don't think this is very clear about the purpose of the flag. How about:

\param IncludeType If set, ensure that the type of the expression printed matches the type of the template argument.

73–81

@rnk Ping.

83–88

This is marked 'done' but not done. Note that plain char is always either a signed or unsigned type, so your check here does not work. You can check T->isSpecificBuiltinType(BuiltinType::SChar) and T->isSpecificBuiltinType(BuiltinType::UChar) instead.

106–110

This isn't correct: u8'x' will print as u8'120'. Perhaps you can factor code to do this properly out of StmtPrinter::VisitCharacterLiteral.

Also, the prefix for char16_t literals is u, not u16, and the prefix for char32_t literals is U, not u32.

431–432

This is marked as done but not done. (The template parameter object case has a FIXME but the other cases do not and should.)

clang/lib/AST/TypePrinter.cpp

2019

Please capitalize this, following the naming convention of the surrounding code.

2027–2028

This is wrong; we'll incorrectly assume that element i of the pack corresponds to element i of the original template parameter list. We should use the same template parameter for all elements of the pack. (For example, you could pass in I and instruct the inner invocation of printTo to not increment I as it goes.)

clang/lib/Sema/SemaDeclCXX.cpp

1000

It doesn't make sense to pass a parameter list in here, because we've not selected a template yet.

1042–1043

In this case, the parameter list is TraitTD->getTemplateParameters().

clang/lib/Sema/SemaTemplateDeduction.cpp

4712–4715

Ping, can this be replaced by a call to printTemplateArgumentList?

[reikdas]

Address @rsmith comments.

[reikdas]

clang/lib/AST/TemplateBase.cpp

106–110

This isn't correct: u8'x' will print as u8'120'. Perhaps you can factor code to do this properly out of StmtPrinter::VisitCharacterLiteral.

I partially addressed this comment. I wasn't able to find a suitable example to test u8'x' being printed as u8'120'. @rsmith could you please help me by showing me a reproducer?

[rsmith]

clang/lib/AST/TemplateBase.cpp

106–110

You'll need a C++20 test (or a test using -fchar8_t). Then:

template<auto> struct A {};
A<u8'x'>::B b; // expected-error {{no type named 'B' in 'A<u8'x'>'}}

[reikdas]

Address @rsmith review

[rsmith]

clang/lib/AST/TemplateBase.cpp

104

I think we should use the prettier printing for wchar_t / char8_t / char16_t / char32_t regardless of whether IncludeType is set, just like we do for char.

106

This has the same problem that u8 handling had: it will print the numeric value not the character.

112–114

You don't need the isUnsignedIntegerType() checks here (4x).

112–115

These two cases have the same problem that u8 handling had: they will print the numeric value not the character.

122

Assuming this is reachable, we should include a cast here.

467–475

In the expression case, we've not resolved the template argument against a parameter, so there's no possibility to include or not include the type. I would remove this FIXME.

clang/lib/AST/TypePrinter.cpp

2027–2028

You need to pass in I here, or we'll assume that all elements of the pack correspond to the first template parameter.

clang/test/CXX/lex/lex.literal/lex.ext/p12.cpp

22

Note the sample output here is wrong. We should ideally print

operator""_x<char32_t, U'\"', U'т', U'е', U'с', U'т', U'_', U'𐀀">

(with or without the \ before the "), but if we don't have a good way to figure out which characters are printable, printing

operator""_x<char32_t, U'\"', U'\u0442', U'\u0435', U'\u0441', U'\u0442', U'_', U'\U00010000">

would be an acceptable fallback. You should check if LLVM already has some way to determine if a given Unicode character is printable and use it if available. I think the diagnostics infrastructure may use something like this already.

clang/test/CXX/lex/lex.literal/lex.ext/p13.cpp

8

(Per earlier comment, I think we should use the prettier printing for char8_t here, regardless of IncludeType.)

clang/test/SemaCXX/cxx1z-ast-print.cpp

9

This line is unmanageably long; please move these expected comments to separate lines. (You can use \ line continuation, or expected-warning@-N, or expected-warning@#foo to expect a diagnostic message on a different line.)

[reikdas]

Address @rsmith comments.

[rsmith]

clang/lib/AST/TemplateBase.cpp

84

getLimitedValue doesn't return a pointer, and the host WCHAR_MAX has no bearing on how target literals should be printed. Please take a look at StmtPrinter::VisitCharacterLiteral and see if you can factor out a helper function that can be shared between that and this. (Maybe as a static member of CharacterLiteral?)

92

This is still printing the numeric value of the character rather than the character itself.

95

Like in the above case, this cast doesn't make sense.

clang/test/CXX/lex/lex.literal/lex.ext/p12.cpp

22

This output is still wrong.

[rnk]

clang/lib/AST/TemplateBase.cpp

73–81

I think we do need to suppress the suffixes for MSVCFormatting. Consider this visualizer I found in the stl.natvis file:

<Type Name="std::chrono::duration&lt;*,std::ratio&lt;1,1000000000&gt; &gt;">
    <DisplayString>{_MyRep} nanoseconds</DisplayString>
    <Expand/>
</Type>

<Type Name="std::chrono::duration&lt;*,std::ratio&lt;1,1000000&gt; &gt;">
    <DisplayString>{_MyRep} microseconds</DisplayString>
    <Expand/>
</Type>

Adding L or ULL after 1000000 has the potential to break the visualizer.

However, in general, I don't think we need to freeze this code in amber. It's not like we put a lot of thought into making this code produce MSVC-idential names when we started using it in CodeView debug info. We just used it and debug issues as they come up.

I think a good rule of thumb for making changes is probably to ask if the main STL data structure names include this template argument feature. So, auto-typed non-type template parameters probably aren't an issue.

---

Separately, I wish the stl.natvis file was part of the github.com/microsoft/STL project so I could just link to it...

[reikdas]

Address @rsmith comments.

[reikdas]

clang/lib/AST/TemplateBase.cpp

73–81

Just to clarify - is an MSVCFormatting specific change required in this patch? (do we need to suppress suffixes and casts for MSVCFormatting printing policy?)
And is there anyone we need to notify about this patch before it lands (to make sure nothing breaks)?

[reikdas]

ping...

[rnk]

clang/lib/AST/TemplateBase.cpp

73–81

Yes, as explained above, this change is likely to break visualizers in Visual Studio. Please preserve the existing behavior of integer printing in debug info when targeting a Windows MSVC environment, and add a test for it.

[reikdas]

Update diff to avoid printing type information if MSVCFormatting printing policy is enabled.

[rnk]

My concerns are addressed, but I think @rsmith hasn't confirmed that all his concerns are addressed yet.

[rsmith]

Thanks, I'm broadly very happy with this. My remaining comments are all very minor.

clang/include/clang/AST/Expr.h

1618

This is sufficiently large that it should be in the .cpp file not in the header. I think calling this print would be more in line with our normal conventions. Please capitalize val as Val.

clang/lib/AST/DeclTemplate.cpp

174

Style nit: no need for else when the if side returns.

clang/lib/AST/TemplateBase.cpp

82–83

Can we use CharacterLiteral::streamChar here too?

clang/lib/AST/TypePrinter.cpp

1996–1998

Please capitalize isPack and parmIndex.

2001

This should have a !isPack condition -- we don't need this because there can't be default arguments for a parameter pack anyway, and we don't want it because it's looking at the wrong parameters in TPL.

[reikdas]

Address @rsmith comments

[rsmith]

Thanks, this looks good to me!

Thank you for your patience :)

[reikdas]

Fix API usage in clangd and fix bug introduced in patch where printing of manually specified default template argument is omitted.

[reikdas]

Fix failing test on windows triple and do some code cleanup.

[dblaikie]

Came across this while trying to do "simplified template names" - producing template names in DWARF without template parameter lists as part of the textual name, then rebuilding that name from the structural representation of template parameters in DWARF (DW_TAG_template_*_parameter, etc). The roundtripping is implemented to ensure that the simplified names are non-lossy - that all the data is still available through the structural representation. (some names are harder or not currently possible to rebuild)

The selective use of suffixes, especially contextually (overloading) seems like something I'll probably want to avoid entirely for DWARF to keep the names consistent across different contexts - but I am also just a bit confused about some things I'm seeing with this change.

For instance, it looks like https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/Decl.cpp#L2900 doesn't pass the list of template parameters, so function template names always get suffixes on their integer parameters.

Whereas the equivalent calls for printing class template specialization names here: https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/DeclTemplate.cpp#L949-L959 - is that just a minor bug/missing functionality?

I'm testing a fix for that:

diff --git clang/lib/AST/Decl.cpp clang/lib/AST/Decl.cpp
index 60ca8633224b..11cf068d2c4c 100644
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2897,7 +2897,7 @@ void FunctionDecl::getNameForDiagnostic(
   NamedDecl::getNameForDiagnostic(OS, Policy, Qualified);
   const TemplateArgumentList *TemplateArgs = getTemplateSpecializationArgs();
   if (TemplateArgs)
-    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy);
+    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy, getPrimaryTemplate()->getTemplateParameters());
 }
 
 bool FunctionDecl::isVariadic() const {

I've sent out a patch with ^ applied/cleanups applied: D77598

But for the debug info, I'm considering changing debug info printing of names to use this:

OS << ND->getDeclName();
printTemplateArgumentList(OS, Args->Args, PP);

Without passing the template parameters/explicitly printing the name separately from the template arguments - but alternatively we could add a PrintingPolicy to address this issue instead. (though I have some reasons to prefer this separated approach - because I want to separate them explicitly for the simplified template name work, for some reasons).

Oh, and here's another case missing its template parameter list: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/NestedNameSpecifier.cpp#L310-L328 - that one probably points to the DWARF usage needing a more complete way to opt out of this - since there might be nested name printing and such that would need to be canonicalized (honestly it points to flaws in my idea even without nested name specifiers -parameters of parameters would still not be handled correctly even if I split up the naming as suggested in the patch above).

@rsmith - any ideas/thoughts/perspectives here?

[v.g.vassilev]

In D77598#3035449, @dblaikie wrote:

Came across this while trying to do "simplified template names" - producing template names in DWARF without template parameter lists as part of the textual name, then rebuilding that name from the structural representation of template parameters in DWARF (DW_TAG_template_*_parameter, etc). The roundtripping is implemented to ensure that the simplified names are non-lossy - that all the data is still available through the structural representation. (some names are harder or not currently possible to rebuild)

The selective use of suffixes, especially contextually (overloading) seems like something I'll probably want to avoid entirely for DWARF to keep the names consistent across different contexts - but I am also just a bit confused about some things I'm seeing with this change.

For instance, it looks like https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/Decl.cpp#L2900 doesn't pass the list of template parameters, so function template names always get suffixes on their integer parameters.

Whereas the equivalent calls for printing class template specialization names here: https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/DeclTemplate.cpp#L949-L959 - is that just a minor bug/missing functionality?

I'm testing a fix for that:

diff --git clang/lib/AST/Decl.cpp clang/lib/AST/Decl.cpp
index 60ca8633224b..11cf068d2c4c 100644
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2897,7 +2897,7 @@ void FunctionDecl::getNameForDiagnostic(
   NamedDecl::getNameForDiagnostic(OS, Policy, Qualified);
   const TemplateArgumentList *TemplateArgs = getTemplateSpecializationArgs();
   if (TemplateArgs)
-    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy);
+    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy, getPrimaryTemplate()->getTemplateParameters());
 }
 
 bool FunctionDecl::isVariadic() const {

I've sent out a patch with ^ applied/cleanups applied: D77598

This seems an oversight on our end, thanks! You probably meant to link https://reviews.llvm.org/D110898

[dblaikie]

In D77598#3035591, @v.g.vassilev wrote:

In D77598#3035449, @dblaikie wrote:

Came across this while trying to do "simplified template names" - producing template names in DWARF without template parameter lists as part of the textual name, then rebuilding that name from the structural representation of template parameters in DWARF (DW_TAG_template_*_parameter, etc). The roundtripping is implemented to ensure that the simplified names are non-lossy - that all the data is still available through the structural representation. (some names are harder or not currently possible to rebuild)

The selective use of suffixes, especially contextually (overloading) seems like something I'll probably want to avoid entirely for DWARF to keep the names consistent across different contexts - but I am also just a bit confused about some things I'm seeing with this change.

For instance, it looks like https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/Decl.cpp#L2900 doesn't pass the list of template parameters, so function template names always get suffixes on their integer parameters.

Whereas the equivalent calls for printing class template specialization names here: https://github.com/llvm/llvm-project/blob/fcdefc8575866a36e80e91024b876937ae6a9965/clang/lib/AST/DeclTemplate.cpp#L949-L959 - is that just a minor bug/missing functionality?

I'm testing a fix for that:

diff --git clang/lib/AST/Decl.cpp clang/lib/AST/Decl.cpp
index 60ca8633224b..11cf068d2c4c 100644
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -2897,7 +2897,7 @@ void FunctionDecl::getNameForDiagnostic(
   NamedDecl::getNameForDiagnostic(OS, Policy, Qualified);
   const TemplateArgumentList *TemplateArgs = getTemplateSpecializationArgs();
   if (TemplateArgs)
-    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy);
+    printTemplateArgumentList(OS, TemplateArgs->asArray(), Policy, getPrimaryTemplate()->getTemplateParameters());
 }
 
 bool FunctionDecl::isVariadic() const {

I've sent out a patch with ^ applied/cleanups applied: D77598

This seems an oversight on our end, thanks! You probably meant to link https://reviews.llvm.org/D110898

Yep, thanks for the correction!

Do you have any interest/bandwidth to look into the one with nested namespaces (the other code I pointed to, in NestedNameSpecifier.cpp, above) case? At a quick glance I wasn't able to quite figure out how to get the template parameters from the TemplateSpecializationType or DependentTemplateSpecializationType to pass in.

[dblaikie]

clang/lib/AST/DeclPrinter.cpp

1101

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>"...

[dblaikie]

clang/lib/AST/DeclPrinter.cpp

1101

Ping on this (posted https://reviews.llvm.org/D111477 for some further discussion on the debug info side of things)

[rsmith]

clang/lib/AST/DeclPrinter.cpp

1101

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.)

[dblaikie]

clang/lib/AST/DeclPrinter.cpp

1101

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.