Integral template argument suffix and cast printing

[reikdas]

Summary

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

[reikdas]

Addresses inline comments.

[reikdas]

Sorry my earlier update did not include the changes from the earlier revision.

[rsmith]

In D77598#1990197, @rsmith wrote:

Is is feasible to check whether the corresponding template parameter either has a deduced type or is a parameter of a function template? If not, we don't need to clarify the type of the template argument and could leave off the suffix to shorten the printed argument.

Ping on this comment. If possible, I'd prefer for us to not produce suffixes and casts in the case where there's no need to clarify the type of the template argument. I would imagine we can pass in a flag into the printing routine to indicate whether it needs to produce an expression with the right type, or merely one convertible to the right type.

clang/lib/AST/TemplateBase.cpp

71–74

It looks like MSVCFormatting wants bool values to be printed as 0 and 1, and this patch presumably changes that (along with the printing of other builtin types). I wonder if this is a problem in practice (eg, if such things are used as keys for debug info or similar)...

82

This isn't correct. Even if the type is canonically a BuiltinType, T might be a type sugar node, in which case this cast will fail at runtime. Instead, use:

if (auto *BT = T->getAs<BuiltinType>()) {
  switch (BT->getKind()) {

or similar.

99–100

This isn't really appropriate for any of the remaining types other than unsigned int. If the type is unsigned but not unsigned int nor any of the cases you handled above, then a U suffix will never produce the right type (a decimal literal with a U suffix always has type unsigned int, unsigned long, or unsigned long long.)

Please convert this to a regular case BuiltinType::UInt: and use a cast for all the remaining cases.

104

Tiny style nit: I'd like to see a break; here.

[reikdas]

Addresses @rsmith 's comments.

[rsmith]

In D77598#2064193, @rsmith wrote:

In D77598#1990197, @rsmith wrote:

Is is feasible to check whether the corresponding template parameter either has a deduced type or is a parameter of a function template? If not, we don't need to clarify the type of the template argument and could leave off the suffix to shorten the printed argument.

Ping on this comment. If possible, I'd prefer for us to not produce suffixes and casts in the case where there's no need to clarify the type of the template argument. I would imagine we can pass in a flag into the printing routine to indicate whether it needs to produce an expression with the right type, or merely one convertible to the right type.

This comment has not been addressed.

clang/lib/AST/TemplateBase.cpp

71–74

Do we need to suppress printing the suffixes below in MSVCFormatting mode too?

80

This looks suspicious: we should make sure we always include an indicator of the type if it was deduced, and this suppresses that. Also you don't print anything at all if the type is not deduced.

[reikdas]

In D77598#2111606, @rsmith wrote:

In D77598#2064193, @rsmith wrote:

In D77598#1990197, @rsmith wrote:

Is is feasible to check whether the corresponding template parameter either has a deduced type or is a parameter of a function template? If not, we don't need to clarify the type of the template argument and could leave off the suffix to shorten the printed argument.

Ping on this comment. If possible, I'd prefer for us to not produce suffixes and casts in the case where there's no need to clarify the type of the template argument. I would imagine we can pass in a flag into the printing routine to indicate whether it needs to produce an expression with the right type, or merely one convertible to the right type.

This comment has not been addressed.

@rsmith Can you elaborate a little on what you meant in your comment? I thought you meant that we should not print the suffixes and casts for the type if it was deduced. Did you instead mean that we should only print the cast and the suffix if the type is a deduced type?

[• Quuxplusone]

clang/test/SemaTemplate/matrix-type.cpp

77 ↗

(On Diff #268412)

Re clarifying Richard's comment: My initial reaction is that every diff in this file is unwanted. The old output was unambiguous, despite lacking suffixes.

However, consider the following silly error message from Clang trunk. This is what we hope to avoid after this patch, I assume:

// https://godbolt.org/z/6nGmkE
template<auto N> struct S {};
template<> struct S<1> { using type = int; };
S<1L>::type t;

error: no type named 'type' in 'S<1>'; did you mean 'S<1>::type'?

clang/test/SemaTemplate/temp_arg_nontype.cpp

280

Re clarifying Richard's comment: I believe that Richard wants this to produce enable_if_unsigned_int<2>'; did you mean instead of enable_if_unsigned_int<2U>'; did you mean.
It looks to me like Clang is producing the shorter version when it prints out the corrected type enable_if_unsigned_int<1>, but not when it prints out the faulty type enable_if_unsigned_int<2U>. So maybe you addressed his comment in one place but not the other?

[reikdas]

Addresses inline comments.

[reikdas]

clang/lib/AST/TemplateBase.cpp

71–74

@rsmith The tests pass, so that is reassuring at least. Is there any other way to find out whether we need to suppress printing the suffixes for other types in MSVCFormatting mode?

[reikdas]

Can completely replace CanonParamType.

[reikdas]

Made some changes to SemaTemplate.cpp based on http://clang-developers.42468.n3.nabble.com/Using-clang-tool-to-Exact-type-names-in-template-specification-arguments-td4069210.html

[rsmith]

clang/lib/AST/TemplateBase.cpp

71–74

@rnk Can you take a look at this? Per rG60103383f097b6580ecb4519eeb87defdb7c05c9 and PR21528 it seems like there might be specific requirements for how template arguments are formatted for CodeView support; we presumably need to make sure we still satisfy those requirements.

clang/lib/Sema/SemaTemplate.cpp

6825–6831

I'm uncomfortable with this approach. It's at least theoretically important that we canonicalize the types of template arguments so that template instantiations don't depend on how their arguments were spelled. Also, I don't think this addresses the whole problem: whether we need the type for a template argument depends not only on whether there was a deduced type in the parameter, but also whether the template was selected from an overload set. That's information that whoever is asking to print the template argument should know, but the template argument itself doesn't know in general.

So I'd like us to approach this a different way: change the calls into the template argument printing code to explicitly pass in a flag that says whether the type of the template argument is known from context. We should pass that in as false when:

1. dumping template arguments
2. the corresponding parameter contains a deduced type
3. the template arguments are for a DeclRefExpr that hadMultipleCandidates()

[reikdas]

Attempt an alternate approach, following @rsmith 's inline suggestions. Unfortunately, one of the tests added in this revision unexpectedly fail in this approach :(

[reikdas]

clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp

466

@rsmith This test fails with this new diff :( Could you please suggest a possible way to fix this?

[rnk]

clang/lib/AST/TemplateBase.cpp

71–74

Probably. This change looks like it preserves behavior from before when MSVCFormatting is set, so I think this is OK. I checked, my version of MSVC still uses 1/0 in debug info for boolean template args.

[rsmith]

Having looked a bit more at the callers here, I think the pattern we're going to want is:

• everywhere that prints a template argument list also asks for a list of corresponding template parameters
• if the list of template parameters is unknown (perhaps because the template name is dependent), or ambiguous from the printed context (because the template was an overloaded function template), then always include types for template arguments
• otherwise, include types for only template arguments where the parameter's type involved a deduced type

On that basis, I think it would be preferable to change TemplateArgument::print to accept not a bool indicating whether the type was known, but instead a const NamedDecl * for the corresponding template parameter. Then the logic is: include the type if we don't know the corresponding parameter or if its type is deduced. And in the caller, we want to always pass in the corresponding parameter if we know it, except when printing the arguments for an overloaded call to a function template (for which the types of the template arguments could affect which overload we selected). [The comments below assume this refactoring is not done.]

clang/include/clang/AST/StmtDataCollectors.td

54 ↗

(On Diff #293432)

We only know the type if the call wasn't overloaded.

clang/include/clang/AST/TemplateBase.h

382 ↗

(On Diff #293432)

Please capitalize this name as KnownType to be consistent with surrounding code.

clang/lib/AST/ASTTypeTraits.cpp

132–136 ↗

(On Diff #293432)

Perhaps we should always request that the type is included when printing a DynTypedNode -- someone printing a DynTypedNode can't be assumed to always know the type of the node in general. But this really depends on the intent of the caller -- if the purpose is to provide text that can be inserted by a refactoring tool, whether we include type information or not will depend on the context where the value is used, not only on the value itself.

I'd prefer that we either always pass in false (given that we don't know whether the type is known in the caller's context) or always pass in true (to preserve the old behavior).

clang/lib/AST/DeclPrinter.cpp

1080 ↗

(On Diff #293432)

What we should do for both this function and the one below is to pass in a flag to indicate whether the template itself (not the template argument) was overloaded, and also pass in the corresponding template parameter list. Then we know the type only if the template was not overloaded and the parameter's type does not contain a deduced type.

clang/lib/AST/DeclTemplate.cpp

1429–1435 ↗

(On Diff #293432)

We should say we know the type here unless the corresponding template parameter has a deduced type.

clang/lib/AST/Expr.cpp

790 ↗

(On Diff #293432)

I think this should be false: generally we want __PRETTY_FUNCTION__ and friends to uniquely identify the function template specialization.

clang/lib/AST/TemplateBase.cpp

55–57

I'd find it much more intuitive to reverse the sense of this flag so that it's an IncludeType flag or similar.

71–74

My concern is that we're changing the behavior for the other cases below in MSVCFormatting mode, not that we're changing the behavior for bool. If we have specific formatting requirements in MSVCFormatting mode, they presumably apply to all types, not only to bool, so we should be careful to not change the output in MSVCFormatting mode for any type.

76–79

We'll need to include a cast or similar here for signed char and unsigned char.

82–88

I don't think we can reliably determine whether the type was originally a deduced type from here; a canonical TemplateArgument doesn't preserve that information. Instead, we should determine whether the type of the corresponding parameter contained a deduced type, in the callers of TemplateArgument::print.

391

Please add a FIXME to handle knownType here.

399

Please add a FIXME to handle knownType here.

clang/lib/AST/TypePrinter.cpp

1789–1793 ↗

(On Diff #293432)

We should pass a corresponding template parameter declaration in here, and pass true for KnownType only if we know the corresponding parameter and it doesn't contain a deduced type.

[reikdas]

Addresses @rsmith 's comments.

On that basis, I think it would be preferable to change TemplateArgument::print to accept not a bool indicating whether the type was known, but instead a const NamedDecl * for the corresponding template parameter. Then the logic is: include the type if we don't know the corresponding parameter or if its type is deduced.

I decided to continue passing a bool and move the logic for checking if the corresponding parameter's type is deduced to the caller. This is to handle the case where we need to let the template argument printing routine know not to print the cast when we get a parameter list whose size is lesser than the length of the list of arguments.

Unfortunately, there are still some cases as seen in the tests where there are explicit casts when there probably shouldn't be.

[rsmith]

clang/include/clang/AST/StmtDataCollectors.td

67–75 ↗

(On Diff #306097)

This case can't happen. The only way that a template parameter can be a TypeDecl is if it's a TemplateTypeParmDecl, which represents TemplateTypeParmType, never a DeducedType.

76 ↗

(On Diff #306097)

Check directly for a NonTypeTemplateParamDecl here.

77 ↗

(On Diff #306097)

Don't use getTypePtr() here, just use QualType's operator->. Eg: IncludeType = ParamValueDecl->getType()->getContainedDeducedType();.

78–84 ↗

(On Diff #306097)

You don't need to check for both of these; AutoType derives from DeducedType. Also, you're only looking for a top-level deduced type here; use getContainedDeducedType() instead to properly handle auto* and the like.

clang/lib/AST/ASTTypeTraits.cpp

131–135 ↗

(On Diff #306097)

No braces here, please.

clang/lib/AST/Decl.cpp

1630 ↗

(On Diff #306097)

Class templates are never overloaded.

2859 ↗

(On Diff #306097)

We don't know in this case, so I think if we're not going to check, then we shouldn't pass the parameter list. This change as-is can lead to us printing out ambiguous names for function templates, both due to argument types and due to omitting arguments equivalent to default arguments.

clang/lib/AST/NestedNameSpecifier.cpp

291 ↗

(On Diff #306097)

This is a class template, not a function template, so can't be overloaded. (Also I think this comment is backwards.)

321 ↗

(On Diff #306097)

Type templates can't be overloaded. We need the types if and only if we didn't resolve the template name to a specific template decl, which matches what this patch does, so you can just remove the FIXMEs.

330–332 ↗

(On Diff #306097)

I think Record will always be null here, so this dyn_cast will crash. For a dependent template specialization type, I don't think there's any situation in which we can know what the template parameter list is, so we should always include the types for template arguments (that is, pass nullptr as the template parameter list).

clang/lib/AST/StmtPrinter.cpp

999 ↗

(On Diff #306097)

I don't think there can ever be such a list: because the scope is dependent, we don't know what template is being named.

1011 ↗

(On Diff #306097)

Similarly here, because the lookup is unresolved, the template parameter list is unknown.

1451 ↗

(On Diff #306097)

The cases you need to check for here are VarTemplateSpecializationDecl and FunctionDecl.

2228 ↗

(On Diff #306097)

We can never have such a list here due to the dependent scope.

2244 ↗

(On Diff #306097)

Likewise.

2326 ↗

(On Diff #306097)

E is a ConceptSpecializationExpr. It can't be a DeclRefExpr. What you want here is TPL = E->getNamedConcept()->getTemplateParameters().

clang/lib/AST/TemplateBase.cpp

77–82

We should not include a cast for plain char, only for signed char and unsigned char.

429

Passing in IncludeType seems appropriate here; I think you can remove the FIXME. Though make sure you have a test for things like:

template<auto ...N> struct X {};
X<1, 1u> x; // should be printed with type suffixes

clang/lib/CodeGen/CGDebugInfo.cpp

281 ↗

(On Diff #306097)

Debug info shouldn't depend on such things. I think we should never pass the template parameter list here, so we always get complete debug information.

1200–1202 ↗

(On Diff #306097)

Likewise here, I think we should always produce complete type names in debug info even though that will be more verbose than necessary to uniquely identify the type.

2107 ↗

(On Diff #306097)

Likewise here

clang/lib/Sema/SemaDeclCXX.cpp

980–1007 ↗

(On Diff #306097)

I think this is around the 6th time this code fragment has appeared in this patch. Would it be possible to factor this out? Perhaps into something like

static bool TemplateParameterList::shouldIncludeTypeForArgument(TemplateParameterList *TPL, unsigned Idx)`

clang/lib/Sema/SemaTemplate.cpp

3451

This can't be overloaded; you can remove this FIXME.

clang/lib/Sema/SemaTemplateDeduction.cpp

4723–4726 ↗

(On Diff #306097)

Hm, are we missing commas between these arguments?

Please consider whether this and some of the other repeated instances of this code can be replaced by calls to printTemplateArgumentList.

clang/lib/Sema/SemaTemplateInstantiate.cpp

591 ↗

(On Diff #306097)

I don't think we should ever pass it in this case; we want to print out at least the values of prior arguments that were instantiated from default arguments.

658–661 ↗

(On Diff #306097)

This one is debatable, but I think we should also never pass the argument list here, and always print out the full description of the function template specialization.

814–818 ↗

(On Diff #306097)

In this case, including substituted default argument values seems important.

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

21 ↗

(On Diff #306097)

It would be useful to generate u8'...', u16'...', and u32'...' literals at least whenever we think they'd contain printable characters.

[reikdas]

Address @rsmith 's comments.

[reikdas]

clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp

473

@rsmith This test fails and we are unable to print the suffix here because the length of the TemplateParameterList is less than the length of the corresponding list of Template Arguments. Do you have any suggestions on how we can fix this?

[rsmith]

clang/include/clang/AST/DeclTemplate.h

216 ↗

(On Diff #307587)

May as well go straight to NonTypeTemplateParamDecl here; it can't be any other kind of ValueDecl.

clang/include/clang/AST/StmtDataCollectors.td

48–72 ↗

(On Diff #307587)

I'm not entirely sure what this is used for, but it looks like the goal is uniqueness, not human-readability, given that the template arguments are separated by newlines not by commas or similar. That being the case, I think we should unconditionally pass true as IncludeType here.

61–64 ↗

(On Diff #307587)

Nit: exactly one space between } and else. (You have two spaces in the first instance here and a newline in the second instance.)

clang/lib/AST/DeclTemplate.cpp

1437 ↗

(On Diff #307587)

Concepts can't be overloaded; remove this FIXME.

clang/lib/AST/Expr.cpp

702 ↗

(On Diff #307587)

Remove this FIXME: class template specializations can't be overloaded.

clang/lib/Sema/SemaTemplateInstantiate.cpp

814–818 ↗

(On Diff #306097)

Sorry, I wasn't clear: I think we should not include the template parameters here, so that we do print out substituted template arguments even if they're equivalent to the default argument.

clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp

473

Once we hit a template parameter pack, we should use the same parameter for all subsequent arguments.

[rsmith]

clang/include/clang/AST/DeclTemplate.h

213 ↗

(On Diff #307587)

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

651–652 ↗

(On Diff #307587)

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

994–995 ↗

(On Diff #307587)

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

clang/lib/AST/StmtPrinter.cpp

1450 ↗

(On Diff #307587)

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

1453 ↗

(On Diff #307587)

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

55

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.

71–74

@rnk Ping.

77–82

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.

103–107

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.

391

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

1980 ↗

(On Diff #307587)

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

1989 ↗

(On Diff #307587)

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

992 ↗

(On Diff #307587)

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

1035 ↗

(On Diff #307587)

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

clang/lib/Sema/SemaTemplateDeduction.cpp

4723–4726 ↗

(On Diff #306097)

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

[reikdas]

Address @rsmith comments.

[reikdas]

clang/lib/AST/TemplateBase.cpp

103–107

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

103–107

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

101

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.

103

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

109–111

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

109–112

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

119

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

417–425

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

1989 ↗

(On Diff #307587)

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

21 ↗

(On Diff #306097)

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

7 ↗

(On Diff #314859)

(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

8 ↗

(On Diff #314859)

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

81

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

89

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

92

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

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

21 ↗

(On Diff #306097)

This output is still wrong.

[rnk]

clang/lib/AST/TemplateBase.cpp

71–74

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

71–74

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

71–74

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

1619 ↗

(On Diff #342991)

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 ↗

(On Diff #342991)

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

clang/lib/AST/TemplateBase.cpp

77–79

Can we use CharacterLiteral::streamChar here too?

clang/lib/AST/TypePrinter.cpp

1996–1997 ↗

(On Diff #342991)

Please capitalize isPack and parmIndex.

2001 ↗

(On Diff #342991)

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 ↗

(On Diff #344897)

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 ↗

(On Diff #344897)

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 ↗

(On Diff #344897)

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 ↗

(On Diff #344897)

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.

clang/lib/AST/TemplateBase.cpp

 ///
 /// \param TemplArg the TemplateArgument instance to print.
 ///
 /// \param Out the raw_ostream instance to use for printing.
 ///
 /// \param Policy the printing policy for EnumConstantDecl printing.
 static void printIntegral(const TemplateArgument &TemplArg,
                           raw_ostream &Out, const PrintingPolicy& Policy) {
   const Type *T = TemplArg.getIntegralType().getTypePtr();
   const llvm::APSInt &Val = TemplArg.getAsIntegral();
 
   if (const EnumType *ET = T->getAs<EnumType>()) {
     for (const EnumConstantDecl* ECD : ET->getDecl()->enumerators()) {
       // In Sema::CheckTemplateArugment, enum template arguments value are
       // extended to the size of the integer underlying the enum type.  This
       // may create a size difference between the enum value and template
       // argument value, requiring isSameValue here instead of operator==.
       if (llvm::APSInt::isSameValue(ECD->getInitVal(), Val)) {
         ECD->printQualifiedName(Out, Policy);
         return;
       }
     }
   }
 
   if (T->isBooleanType() && !Policy.MSVCFormatting) {
     Out << (Val.getBoolValue() ? "true" : "false");
+  } else if (T->isBooleanType()) {
+    Out << Val;
   } else if (T->isCharType()) {
     const char Ch = Val.getZExtValue();
     Out << ((Ch == '\'') ? "'\\" : "'");
     Out.write_escaped(StringRef(&Ch, 1), /*UseHexEscapes=*/ true);
     Out << "'";
   } else {
+    bool flag = false;
+    if (auto *autoT = T->getAs<AutoType>()) {
+      if (autoT->getAs<DeducedType>()) {
+        flag = true;
+      }
+    } else if (T->getAs<DeducedType>()) {
+      flag = true;
+    }
+    if (flag) {
+      if (auto *BT = T->getAs<BuiltinType>()) {
+        switch (BT->getKind()) {
+        case BuiltinType::ULongLong:
+          Out << Val << "ULL";
+          break;
+        case BuiltinType::LongLong:
+          Out << Val << "LL";
+          break;
+        case BuiltinType::ULong:
+          Out << Val << "UL";
+          break;
+        case BuiltinType::Long:
+          Out << Val << "L";
+          break;
+        case BuiltinType::UInt:
+          Out << Val << "U";
+          break;
+        case BuiltinType::Int:
+          Out << Val;
+          break;
+        default:
+          Out << "(" << T->getCanonicalTypeInternal().getAsString(Policy) << ")"
+              << Val;
+          break;
+        }
+      }
+    } else
       Out << Val;
   }
 }
 
 //===----------------------------------------------------------------------===//
 // TemplateArgument Implementation
 //===----------------------------------------------------------------------===//
 
 TemplateArgument::TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value,
                                    QualType Type) {
   Integer.Kind = Integral;
void TemplateArgument::print(const PrintingPolicy &Policy,
 
   case Type: {
     PrintingPolicy SubPolicy(Policy);
     SubPolicy.SuppressStrongLifetime = true;
     getAsType().print(Out, SubPolicy);
     break;
   }
 
   case Declaration: {
     NamedDecl *ND = getAsDecl();
     if (!getParamTypeForDecl()->isReferenceType())
       Out << '&';
     ND->printQualifiedName(Out);
     break;
   }
 
   case NullPtr:
     Out << "nullptr";
     break;
 
   case Template:
     getAsTemplate().print(Out, Policy);
     break;
 
   case TemplateExpansion:
     getAsTemplateOrTemplatePattern().print(Out, Policy);
     Out << "...";
     break;
 
   case Integral:
     printIntegral(*this, Out, Policy);
     break;
 
   case Expression:
     getAsExpr()->printPretty(Out, nullptr, Policy);
     break;
 
   case Pack:
     Out << "<";
     bool First = true;
     for (const auto &P : pack_elements()) {
       if (First)
         First = false;
       else
         Out << ", ";
 
       P.print(Policy, Out);
     }
     Out << ">";
     break;
   }
 }
 
 void TemplateArgument::dump(raw_ostream &Out) const {
   LangOptions LO; // FIXME! see also TemplateName::dump().

clang/lib/Sema/SemaTemplate.cpp

if (DR && DR->getQualifier()) {
       // If this is a qualified name, expand the template arguments in nested
       // qualifiers.
       DR->getQualifier()->print(OS, Policy, true);
       // Then print the decl itself.
       const ValueDecl *VD = DR->getDecl();
       OS << VD->getName();
       if (const auto *IV = dyn_cast<VarTemplateSpecializationDecl>(VD)) {
         // This is a template variable, print the expanded template arguments.
         printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
       }
       return true;
     }
     return false;
   }
 
 private:
   const PrintingPolicy Policy;
if (getLangOpts().CPlusPlus17) {
     // permitted (and expected) to be unable to determine a value.
     if (ArgResult.get()->isValueDependent()) {
       Converted = TemplateArgument(ArgResult.get());
       return ArgResult;
     }
 
     QualType CanonParamType = Context.getCanonicalType(ParamType);
 
+    // Note: this renders CanonParamType non-canonical, but since every
+    // instantiation of this argument will be wrapped in an AutoType (since
+    // Param->getType() will always be an AutoType for this template), there
+    // should be no difference in how arguments are distinguished.
+    if (Param->getType()->getAs<AutoType>())
+      CanonParamType = Context.getAutoType(CanonParamType,
+                                           AutoTypeKeyword::Auto, false, false);
+
     // Convert the APValue to a TemplateArgument.
     switch (Value.getKind()) {
     case APValue::None:
       assert(ParamType->isNullPtrType());
       Converted = TemplateArgument(CanonParamType, /*isNullPtr*/true);
       break;
     case APValue::Indeterminate:
       llvm_unreachable("result of constant evaluation should be initialized");

clang/test/SemaTemplate/temp_arg_nontype.cpp

namespace PR9227 {
 
   template <char C> struct enable_if_char { };
   template <> struct enable_if_char<'a'> { typedef int type; }; // expected-note 5{{'enable_if_char<'a'>::type' declared here}}
   void test_char_0() { enable_if_char<0>::type i; } // expected-error{{enable_if_char<'\x00'>'; did you mean 'enable_if_char<'a'>::type'?}}
   void test_char_b() { enable_if_char<'b'>::type i; } // expected-error{{enable_if_char<'b'>'; did you mean 'enable_if_char<'a'>::type'?}}
   void test_char_possibly_negative() { enable_if_char<'\x02'>::type i; } // expected-error{{enable_if_char<'\x02'>'; did you mean 'enable_if_char<'a'>::type'?}}
   void test_char_single_quote() { enable_if_char<'\''>::type i; } // expected-error{{enable_if_char<'\''>'; did you mean 'enable_if_char<'a'>::type'?}}
   void test_char_backslash() { enable_if_char<'\\'>::type i; } // expected-error{{enable_if_char<'\\'>'; did you mean 'enable_if_char<'a'>::type'?}}
+
+  template <int N> struct enable_if_int {};
+  template <> struct enable_if_int<1> { typedef int type; }; // expected-note{{'enable_if_int<1>::type' declared here}}
+  void test_int() { enable_if_int<2>::type i; } // expected-error{{enable_if_int<2>'; did you mean 'enable_if_int<1>::type'?}}
+
+  template <unsigned int N> struct enable_if_unsigned_int {};
+  template <> struct enable_if_unsigned_int<1> { typedef int type; }; // expected-note{{'enable_if_unsigned_int<1>::type' declared here}}
+  void test_unsigned_int() { enable_if_unsigned_int<2>::type i; } // expected-error{{enable_if_unsigned_int<2>'; did you mean 'enable_if_unsigned_int<1>::type'?}}
+
+  template <unsigned long long N> struct enable_if_unsigned_long_long {};
+  template <> struct enable_if_unsigned_long_long<1> { typedef int type; }; // expected-note{{'enable_if_unsigned_long_long<1>::type' declared here}}
+  void test_unsigned_long_long() { enable_if_unsigned_long_long<2>::type i; } // expected-error{{enable_if_unsigned_long_long<2>'; did you mean 'enable_if_unsigned_long_long<1>::type'?}}
+
+  template <long long N> struct enable_if_long_long {};
+  template <> struct enable_if_long_long<1> { typedef int type; }; // expected-note{{'enable_if_long_long<1>::type' declared here}}
+  void test_long_long() { enable_if_long_long<2>::type i; } // expected-error{{enable_if_long_long<2>'; did you mean 'enable_if_long_long<1>::type'?}}
+
 }
 
 namespace PR10579 {
   namespace fcppt
   {
     namespace container
     {
       namespace bitfield

clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp

template<auto (*f)() -> auto> struct X { // expected-note {{here}}
     auto call() { return f(); }
   };
   X<nullptr> x; // expected-error {{incompatible initializer}}
 
   void *f();
   X<f> y;
   int n = y.call(); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
 }
+
+namespace PR9227 {
+  template <auto N> struct S {};
+  template <> struct S<1> { using type = int; }; // expected-note {{'S<1>::type' declared here}}
+  S<1L>::type t; // expected-error {{no type named 'type' in 'PR9227::S<1L>'; did you mean 'S<1>::type'?}}
+
+  template <auto N> struct A {};
+  template <> struct A<1> { using type = int; }; // expected-note {{'A<1>::type' declared here}}
+  A<2>::type x; // expected-error {{no type named 'type' in 'PR9227::A<2>'; did you mean 'A<1>::type'?}}
+}