Integral template argument suffix and cast printing

[reikdas]

Summary

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

[reikdas]

clang/lib/AST/TemplateBase.cpp

74–77

@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

74–77

@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

6939–6945

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

475

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

[rnk]

clang/lib/AST/TemplateBase.cpp

74–77

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

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

clang/include/clang/AST/TemplateBase.h

393

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

clang/lib/AST/ASTTypeTraits.cpp

136–140

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

1093–1098

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

1438–1439

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

clang/lib/AST/Expr.cpp

725

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

54–56

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

74–77

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.

79–88

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

131–137

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.

408–409

Please add a FIXME to handle knownType here.

423–424

Please add a FIXME to handle knownType here.

clang/lib/AST/TypePrinter.cpp

1852–1853

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

62–70

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.

71

Check directly for a NonTypeTemplateParamDecl here.

72

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

73–79

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

135–136

No braces here, please.

clang/lib/AST/Decl.cpp

1630

Class templates are never overloaded.

2870

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–293

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

320

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.

328–330

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

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.

1010

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

1449

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

2232

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

2247

Likewise.

2328

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

clang/lib/AST/TemplateBase.cpp

80–85

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

463

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

282

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.

1198–1200

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.

2102

Likewise here

clang/lib/Sema/SemaDeclCXX.cpp

980–984

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

3564–3566

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

clang/lib/Sema/SemaTemplateDeduction.cpp

4704–4707

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

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.

656–659

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.

808–809

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

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

21

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

482

@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

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.

216

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

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.

56–59

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/ASTContext.cpp

7463

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

clang/lib/AST/DeclPrinter.cpp

651–652

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

992–995

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

clang/lib/AST/DeclTemplate.cpp

1436

Concepts can't be overloaded; remove this FIXME.

clang/lib/AST/Expr.cpp

707–709

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

clang/lib/AST/StmtPrinter.cpp

1450

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

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.

74–77

@rnk Ping.

80–85

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.

112–116

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.

408–409

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

2014

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

2023–2024

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

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

1034–1035

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

clang/lib/Sema/SemaTemplateDeduction.cpp

4704–4707

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

clang/lib/Sema/SemaTemplateInstantiate.cpp

808–809

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

482

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

[reikdas]

Address @rsmith comments.

[reikdas]

clang/lib/AST/TemplateBase.cpp

112–116

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

112–116

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