This is an archive of the discontinued LLVM Phabricator instance.

Integral template argument suffix and cast printing
ClosedPublic

Authored by reikdas on Apr 6 2020, 3:16 PM.

Details

Summary

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
reikdas added a comment.EditedMay 4 2020, 3:26 AM

Addresses inline comments.

reikdas updated this revision to Diff 261774.May 4 2020, 3:31 AM

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

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
73–76

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

84

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.

101–102

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.

106

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

reikdas updated this revision to Diff 268412.Jun 4 2020, 3:57 AM
reikdas marked an inline comment as done.
reikdas retitled this revision from Integral template argument suffix printing to Integral template argument suffix and cast printing.
reikdas edited the summary of this revision. (Show Details)

Addresses @rsmith 's comments.

reikdas marked 4 inline comments as done.Jun 23 2020, 11:37 PM

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
73–76

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

82

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.

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 added inline comments.
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 updated this revision to Diff 284181.Aug 9 2020, 3:04 AM
reikdas edited the summary of this revision. (Show Details)

Addresses inline comments.

reikdas marked 2 inline comments as done.Aug 9 2020, 3:05 AM
reikdas added inline comments.Aug 9 2020, 3:08 AM
clang/lib/AST/TemplateBase.cpp
73–76

@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 updated this revision to Diff 284186.Aug 9 2020, 4:08 AM

Can completely replace CanonParamType.

rsmith added a reviewer: rnk.Sep 17 2020, 2:10 PM
rsmith added inline comments.Sep 17 2020, 2:21 PM
clang/lib/AST/TemplateBase.cpp
73–76

@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 updated this revision to Diff 293432.Sep 22 2020, 6:15 AM

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

reikdas added inline comments.Sep 22 2020, 6:16 AM
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 added inline comments.Sep 29 2020, 11:13 AM
clang/lib/AST/TemplateBase.cpp
73–76

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 added a comment.Nov 5 2020, 2:14 PM

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
382

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

clang/lib/AST/ASTTypeTraits.cpp
132–136

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

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

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

clang/lib/AST/Expr.cpp
790

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.

73–76

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.

78–81

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

84–90

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.

395

Please add a FIXME to handle knownType here.

403

Please add a FIXME to handle knownType here.

clang/lib/AST/TypePrinter.cpp
1789–1793

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 updated this revision to Diff 306059.EditedNov 18 2020, 5:06 AM

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.

reikdas marked 12 inline comments as done.Nov 18 2020, 5:16 AM
reikdas updated this revision to Diff 306097.Nov 18 2020, 6:53 AM
reikdas marked 2 inline comments as done.
rsmith added inline comments.Nov 18 2020, 11:21 AM
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
131–140

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
79–84

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

433

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
966–971

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

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

clang/lib/Sema/SemaTemplateDeduction.cpp
4702–4705

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 updated this revision to Diff 307587.Nov 25 2020, 5:06 AM

Address @rsmith 's comments.

reikdas marked 25 inline comments as done.Nov 25 2020, 5:14 AM
reikdas added inline comments.
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?

reikdas marked 2 inline comments as done.Nov 25 2020, 5:17 AM
rsmith added inline comments.Dec 9 2020, 1:36 PM
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

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/DeclTemplate.cpp
1429

Concepts can't be overloaded; remove this FIXME.

clang/lib/AST/Expr.cpp
774

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 added inline comments.Dec 9 2020, 1:36 PM
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
647–648

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

983–984

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

@rnk Ping.

79–84

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.

105–109

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.

395

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
1805

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

1813

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
979

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

1021

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

clang/lib/Sema/SemaTemplateDeduction.cpp
4702–4705

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

reikdas updated this revision to Diff 312980.Dec 20 2020, 7:54 AM

Address @rsmith comments.

reikdas marked 22 inline comments as done.Dec 20 2020, 8:04 AM
reikdas added inline comments.
clang/lib/AST/TemplateBase.cpp
105–109

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 added inline comments.Jan 5 2021, 12:59 PM
clang/lib/AST/TemplateBase.cpp
105–109

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 updated this revision to Diff 314858.Jan 6 2021, 4:48 AM
reikdas marked an inline comment as done.

Address @rsmith review

reikdas updated this revision to Diff 314859.Jan 6 2021, 4:54 AM
reikdas marked 2 inline comments as done.
rsmith added inline comments.Feb 8 2021, 12:00 PM
clang/lib/AST/TemplateBase.cpp
103

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.

105

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

111–113

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

111–114

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

121

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

421–429

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
1813

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 updated this revision to Diff 323696.Feb 15 2021, 1:57 AM

Address @rsmith comments.

reikdas marked 9 inline comments as done.Feb 15 2021, 1:59 AM
baziotis removed a subscriber: baziotis.Feb 15 2021, 3:36 AM
rsmith added inline comments.Feb 19 2021, 3:33 PM
clang/lib/AST/TemplateBase.cpp
83

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

91

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

94

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 added inline comments.Feb 22 2021, 12:15 PM
clang/lib/AST/TemplateBase.cpp
73–76

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 updated this revision to Diff 336604.Apr 10 2021, 4:10 AM

Address @rsmith comments.

reikdas marked 5 inline comments as done.Apr 10 2021, 4:12 AM
reikdas added inline comments.Apr 10 2021, 4:15 AM
clang/lib/AST/TemplateBase.cpp
73–76

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

rnk added inline comments.Apr 27 2021, 11:01 AM
clang/lib/AST/TemplateBase.cpp
73–76

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 updated this revision to Diff 342991.May 5 2021, 4:25 AM

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

reikdas marked an inline comment as done.May 5 2021, 4:25 AM
rnk added a comment.May 5 2021, 2:43 PM

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

rsmith added a comment.May 5 2021, 6:28 PM

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
188

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

clang/lib/AST/TemplateBase.cpp
79–81

Can we use CharacterLiteral::streamChar here too?

clang/lib/AST/TypePrinter.cpp
1797–1798

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.

1797–1798

Please capitalize isPack and parmIndex.

reikdas updated this revision to Diff 343363.May 6 2021, 5:06 AM

Address @rsmith comments

reikdas marked 5 inline comments as done.May 6 2021, 5:10 AM
rsmith accepted this revision.May 6 2021, 3:15 PM

Thanks, this looks good to me!

Thank you for your patience :)

This revision is now accepted and ready to land.May 6 2021, 3:15 PM
reikdas updated this revision to Diff 343909.May 9 2021, 6:36 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2021, 6:36 AM
reikdas updated this revision to Diff 344161.May 10 2021, 12:49 PM

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

This revision was landed with ongoing or failed builds.May 12 2021, 12:00 PM
This revision was automatically updated to reflect the committed changes.

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?

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

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 added inline comments.Oct 1 2021, 11:16 PM
clang/lib/AST/DeclPrinter.cpp
1080

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 added inline comments.Oct 10 2021, 9:36 AM
clang/lib/AST/DeclPrinter.cpp
1080

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

rsmith added inline comments.Nov 12 2021, 4:11 PM
clang/lib/AST/DeclPrinter.cpp
1080

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 added inline comments.Nov 14 2021, 9:13 PM
clang/lib/AST/DeclPrinter.cpp
1080

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.