[P0857R0 Part B] Resubmit an implemention for constrained template template parameters

[lime]

Summary

This function had been submitted by @erichkeane long time ago. However, he has not been working on it. So I submit a new patch with some minor differences.

[cor3ntin]

Thanks for working on this.
I'll be honest though, I still have absolutely no understanding what the use cases or intents of this features are. I think we were waiting for core to clarify and I'm not sure they did.
This does seem to implement the wording though...

[lime]

In D134128#3798127, @cor3ntin wrote:

Thanks for working on this.
I'll be honest though, I still have absolutely no understanding what the use cases or intents of this features are. I think we were waiting for core to clarify and I'm not sure they did.
This does seem to implement the wording though...

Perhaps, the intents of this feature are a little confusing, so I add S5 in the file p3-2a.cpp. If it was necessary to check the constrains on the template template parameter, we could expect an error there.

But one intent might be a mend of the tailing syntax about constrains, as a template parameter like template <C> typename is already accepted by Clang.

clang/lib/Parse/ParseTemplate.cpp

882

This is copied from Parser::ParseTemplateDeclarationOrSpecialization.

909

It is fine to skip the check for the language option of C++20. The parser will emit an error here and complain the lack of class or typename, if the option is not provided.

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

36

Test whether Clang behaves the same here as it is on S1.

[erichkeane]

Thanks for taking this over! FIRST, please make sure you re-submit this with the entire 'context' (see context-not-available), by making sure you use -U999999 on your patch before uploading it.

It DOES appear from the tests that we're already checking everything we're supposed to?

ALSO, could you please try rebasing this on top of "https://reviews.llvm.org/D126907" to see if it causes any further issues for that? I'm nearing a solution to my last problem on that, and would love to get that submitted in the next week or two, and knowing if this/these tests are going to break it would be an appreciated heads-up.

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

39

I realize the rest of the test does a poor job of this, but can you better arrange the new diagnostics for me, rather thanputting the 'note's on teh individual lines?

I prefer to have them in 'emitted order' next to the error/warning that caused the notes. You can do an "@" on 'expected-note' that refers to a different line by putting a // #SOME_BOOKMARK_NAME on the line that has the note, and expected-note@#SOME_BOOKMARK_NAME here.

[erichkeane]

One test that i need to have (that might actually end up conflicting with the above mentioned), is a reproducer that has a ClassTemplateDecl that is its own friend. So something like:

template<template <typename> class C requires ...>
struct S {
    template<template <typename> class C requires ...>
    friend struct S;
};

And make sure that an instantiation still checks those correctly (that is, I don't expect any diagnostics).

[lime]

I have not looked deep into D126907, but the rule it referred seems related to something as follows:

template <typename, typename> concept C = true;

template <typename T>
struct S1 {
    template <template <C<T> U> typename> friend void foo() {}
};

// Does the rule say these two functions are not the same?
template <typename T>
struct S2 {
    template <template <C<T> U> typename> friend void foo() {}
};

BTW, I could not image a non-template friend declaration with a requires-clause...

[erichkeane]

In D134128#3805675, @lime wrote:

I have not looked deep into D126907, but the rule it referred seems related to something as follows:

You shouldn't have to look 'deep' into it, just rebase and see what happens. Note that it has not been committed yet, so you'll have to manually apply it.

template <typename, typename> concept C = true;

template <typename T>
struct S {
    template <template <C<T> U> typename> friend void foo(S) {}
};

// Does the rule say these two are not the same?
template <template <C<int> T> typename> S<int> foo(S<int> s) { return s; }

Right, those are not the same because of: https://eel.is/c++draft/temp.friend#9

[erichkeane]

In D134128#3805675, @lime wrote:

I have not looked deep into D126907, but the rule it referred seems related to something as follows:

template <typename, typename> concept C = true;

template <typename T>
struct S1 {
    template <template <C<T> U> typename> friend void foo() {}
};

// Does the rule say these two functions are not the same?
template <typename T>
struct S2 {
    template <template <C<T> U> typename> friend void foo() {}
};

BTW, I could not image a non-template friend declaration with a requires-clause...

You edited while I was answering, but I believe the answer is the same: those are not the same thanks to temp.friend p9.

That D126907 is actually NOT just the friends, this is an entire omnibus "fix the deferred concepts implementation". The original concepts implementation used a greedy instantiation mechanism, which is unfortunately not permitted by the standard. This patch is delaying the concept instantiation until it is required for checking. Along the way that 'friend' example was run across.

[lime]

Well, Something happened after rebasing this patch on D126907. s41 below was rejected as the constrain generated from template <C> was no longer considered to subsume the constrain generated from template <typename T> requires C in the template template argument, which is not the case in both GCC and MSVC. However, GCC and MSVC also accept the redeclaration for S, which might be ill-formed because of this rule. If this kind of redeclaration happens on X, GCC and MSVC will reject it. Rebasing this patch on D126907 will also not make the both redeclaration valid.

Personally, I decided to make s41 valid for Clang, a clue might be making the QualTypes the same in the parameters of two generated constrains.

template <typename T> concept C = T::f();

template <typename T> concept C1 = T::f();

template <C> struct X {};

template <typename T> requires C<T> struct X; // ill-formed for sure

template <C1> struct Y {};

template <template <typename T> requires C<T> class> struct S {};

template <template <C> class> struct S; // GCC and MSVC accept this

S<X> sx; // my target
S<Y> sy; // ill-formed for sure

[erichkeane]

In D134128#3813708, @lime wrote:

Well, Something happened after rebasing this patch on D126907. s41 below was rejected as the constrain generated from template <C> was no longer considered to subsume the constrain generated from template <typename T> requires C in the template template argument, which is not the case in both GCC and MSVC. However, GCC and MSVC also accept the redeclaration for S, which might be ill-formed because of this rule. If this kind of redeclaration happens on X, GCC and MSVC will reject it. Rebasing this patch on D126907 will also not make the both redeclaration valid.

Personally, I decided to make s41 valid for Clang, a clue might be making the QualTypes the same in the parameters of two generated constrains.

template <typename T> concept C = T::f();

template <typename T> concept C1 = T::f();

template <C> struct X {};

template <typename T> requires C<T> struct X; // ill-formed for sure

template <C1> struct Y {};

template <template <typename T> requires C<T> class> struct S {};

template <template <C> class> struct S; // GCC and MSVC accept this

S<X> sx; // my target
S<Y> sy; // ill-formed for sure

I'm not sure I get the issue here, there are some identifiers you're using that don't make sense to me. I don't see that test in the lit tests either, could you make sure it is reflected there and commented? ALso, can you re-arrange the error messages as I requested?

[lime]

I rearranged the error messages. But after I rebased this patch on D126907, the file p3-2a.cpp no longer passed. So I did some investigations there. The problem has not been solved yet.

[lime]

I'm wondering whether classes like TemplateArgumentLoc could refer to the template head of the TemplateArgument, so the comparison of parameter mappings could be modified, and then the referred variable could be accepted.

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

43

This variable was rejected after rebasing the patch on D126907. The reason is that the atomic constraint generated from S4 is not considered to subsume the one generated from X. And the difference between two atomic constraints is mainly the template arguments. If the concept C was like template <typename> concept C = true, this variable would be accepted.

A reasonable behavior might be either accepting the variable regardless of whether the constraint expression depends on template arguments, or rejecting the variable as the template heads of S4 and X are not equivalent. Both GCC and MSVC accept the variable.

[lime]

I provided a solution for the problem I mentioned above, although it may look ugly. Then, I will take a look at friend related stuffs.

[erichkeane]

clang/include/clang/Sema/SemaConcept.h

51 ↗

(On Diff #465395)

what do you mean by 'tailing' here?

58 ↗

(On Diff #465395)

What exactly is going on here? I would expect the 'AdjustConstraintDepth' work to have already made these basically equal.

[lime]

Thanks for the advice. I used AdjustConstraintDepth before subsume. The adjusted constraints are assigned to new addresses, but I think MutableArrayRef could be used here.

what do you mean by 'tailing' here?

I wanted to express the require clauses, as they are stored in TrailingObject.

What exactly is going on here? I would expect the 'AdjustConstraintDepth' work to have already made these basically equal.

AdjustConstraintDepth works here, but I missed it before you mentioned it.

I'm sorry that this update is broken. Some cases are crashed, and s11 in p3-2a.cpp is rejected. Perhaps, I should place AdjustConstraintDepth in another place rather than IsAtLeastAsConstrained, as the depths in Exprs already equal to each other, but the calculations from NamedDecls do not when it comes to s11.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

614

SemaRef or S is our common nomenclature.

[lime]

I updated the patch to fix the previous problem about failing to pass unit tests. And, isn't this patch accepted a little quickly? BTW, NormalizationCache becomes heavier than before.

[erichkeane]

In D134128#3840232, @lime wrote:

I updated patch to fix the previous problem that failed to pass unit tests. And, isn't this patch accepted a little quickly? BTW, NormalizationCache becomes heavier than before.

Huh, woah, Phab didn't show me 1/2 this patch the last time. The changes in Sema.h and the changes of CalculateTemplateDepthForConstraints just weren't there...

So yeah, I guess, spend more time on this?

[lime]

The changes in Sema.h and the changes of CalculateTemplateDepthForConstraints

I think it might be fine now. The above changes are related to NormalizationCache. This cache used to take NamedDecl as its key. However, constraints need to be adjusted to different depths, and p3-2a.cpp can reproduce the case. Thus, NameDecl is not enough, and a heavier key is used. But the needs to adjust the depth seem unique to template-template parameter, so I'm not sure whether there is a better solution. Another performance issue is about IsAtLeastAsConstrained. This function takes ArrayRef as its argument, which leads to new spaces for potentially adjusted constraints. I think MutableArryRef could be used here. At last, CalculateTemplateDepthForConstraints may be incomplete, as I don't know all the possibilities about getting depths from constraint expressions. The constraint in template<template <C T> class> has a depth equal to 0. Thus, calculating from NamedDecl is unreliable here.

BTW, this comment includes an update that involves git clang-format.

[lime]

I updated the patch as I said, while kept the key of NormalizationCache relatively heavy. That means I:

• changed the parameter of IsAtLeastAsConstrained to MutableArrayRef,
• and let the new CalculateTemplateDepthForConstraints handle BinaryOperator and ParenExpr.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

590

I'd like some sort of assert in the case where you don't know what to do here. We need to collect all of these cases best we can in advance.

I'd suggest ALSO running libcxx tests against this once you have the assert in place, which should help.

663

Unless I'm missing something, I don't think this idea of 'unify constraint depth' is correct. We should be able, from the decl itself, to determine the depths? What is the difference here that I'm not getting?

1268

Can you explain why this is a MutableArrayRef now? I believe this means it'll now modify the arrays that are passed into it, which we don't necessarily want, right? A new

[lime]

Unless I'm missing something, I don't think this idea of 'unify constraint depth' is correct. We should be able, from the decl itself, to determine the depths? What is the difference here that I'm not getting?

It is explained by the inline comments in sequence.

Can you explain why this is a MutableArrayRef now? I believe this means it'll now modify the arrays that are passed into it, which we don't necessarily want, right?

In the previous update, I mentioned using ArrayRef is not as efficient as MutableArrayRef. And there was no feedback on my comment for several days, so I changed it as I said. Additionally, I found some code once called IsAtLeastAsConstrained for two constraints, and then call IsAtLeastAsConstrained again with the sequence of the two constraints swapped. So using MutableArrayRef also saves a potential adjustment.
Any way, adjusting depths here might be unique to template template parameters. If so, parsing the require clause in the unit test with depth equal to 0 should be a better solution, and things about CalculateTemplateDepthForConstraints and ArrayRef could remain unchanged.

clang/lib/Sema/SemaConcept.cpp

581

1. The orignal CalculateTemplateDepthForConstraints calculates the depth from NamedDecl.

621

2. And it was only called in AreConstraintExpressionsEqual.

663

3. I extracted the calls to the original CalculateTemplateDepthForConstraints as UnifyConstraintDepthFromDecl. Calculating from a declaration is indeed not accurate, as it returns 1 for template<template <C T> class>, but the depth in the constraint is actually 0. This is one reason why a previous version of patch breaks some unit tests. But I leaves it to keep the code unchanged functionally.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

663

Hmm... SO it seems based on debugging that the issue is exclusive with template template parameters. It DOES make sense that the inner-template is 'deeper', but I think the problem is that getTemplateInstantiationArgs isn't correctly handling a ClassTemplateDecl, so the "D2" in IsAtLeastAsConstrainedAs is returning 0, instead of 1.

The "Depth" of something is the 'number of layers of template arguments'. So for: 'template<template <C T> class>', the answer SHOULD be 1.

So I think the problem is basically that for X, the answer should ALSO be 1 (since it has template parameters), but we aren't handling it. So I think we just need to correctly just call the ClassTemplateDecl::getTemplatedDecl at one point on that one.

1268

I see your response... I am a bit concerned about the use of this in SemaTemplate, which re-uses the generated array after this however (though for diagnostics?).

Everywhere else doesn't seem to be using the underlying array after the fact.

[lime]

clang/lib/Sema/SemaConcept.cpp

663

I guess there could be another patch improving the calculation about depth. In this patch, I just adjust the calculation to pass unit tests about template template parameters.

1268

The function MaybeEmitAmbiguousAtomicConstraintDiagnostic calls subsume on AtomicConstraint without adjusting depths. So, I guess it is fine to reuse the unified arrays.

[lime]

clang/lib/Sema/SemaConcept.cpp

590

I'm afraid that assertions would abort Clang on many code. This function serves as a check for AdjustConstraintDepth, and this class looks like just handling TemplateTypeParmType. The function has already handled TemplateTypeParmType. And determining cases that should trigger the assertions also involves the subsumption of atomic constraints. It is needed to check the depth according to TemplateTypeParmType, because the identity of QualType involves the depth, and then effects the subsumption. There will be some works to determine which cases are the same, and leave assertions for the cases. This kind of works is highly related to the depth and the subsumption, while I think it is less related to template template parameters.

[erichkeane]

clang/lib/Sema/SemaConcept.cpp

590

The point of an assertion would be to do just that! We WANT to make sure we get all those assertions. That said, I'm unconvinced that this code here is necessary.

663

Right, but I'm saying how you're doing it is incorrect and will likely result in incorrect compilations

[lime]

I think I located the problem. In the line 4014 of the file SemaTemplateInstantiateDecl.cpp, the requires clause is not instantiated as the parameters of the template parameter list, and these parameters have been instantiated with a shallower depth. So that's the reason why the depths are not confirm between the T in template <template <C T> class> and the constraints on the instantiated parameters. I guess adjusting depths should be unnecessary in IsAtLeastAsConstrained, if the requires clause was correctly instantiated, so should changes related to SemaConcept.cpp. I will work on this later.

[erichkeane]

In D134128#3860993, @lime wrote:

I think I located the problem. In the line 4014 of the file SemaTemplateInstantiateDecl.cpp, the requires clause is not instantiated as the parameters of the template parameter list, and these parameters have been instantiated with a shallower depth. So that's the reason why the depths are not confirm between the T in template <template <C T> class> and the constraints on the instantiated parameters. I guess adjusting depths should be unnecessary in IsAtLeastAsConstrained, if the requires clause was correctly instantiated, so should changes related to SemaConcept.cpp. I will work on this later.

I wouldn't expect the requires clause (nor any other concept related AST node) to be instantiated at all until it is going through 'checking', which is the purpose of the deferred concepts patch. That said, SemaTemplateInstantiateDecl.cpp:4014 is in the middle of adjustForRewrite, which isn't clear to me what you mean.

That said, I'm not sure what the

[lime]

which isn't clear to me what you mean

• In the function Sema::CheckTemplateArgument at line 5725, Params has been substituted in a way all TemplateTypeParmDecls are instantiated with a smaller depth, and so are constraints of them at SemaTemplateInstantiateDecl.cpp:2769, while the requires clause remains the same.
• Then CheckTemplateArgument calls CheckTemplateTemplateArgument which calls IsAtLeastAsConstrained with the original declaration and the constraint collected from Params. Thus, in IsAtLeastAsConstrained, the depth calculated from the declaration will not reflect the depth in the constraint.
• It should be fine to not adjust the depths between two constraints passed to IsAtLeastAsConstrained if the requires clause is not parsed, as they are already the same. But it is not the case when the constraint is from a requires clause, as requires clauses are not substituted. However, calculating from declarations will break the original case.

I wouldn't expect the requires clause (nor any other concept related AST node) to be instantiated at all until it is going through 'checking'

The function TemplateDeclInstantiator::SubstTemplateParams instantiates constraints like the one template <template <C T> class>, as I mentioned above. So it is already happened.

[erichkeane]

In D134128#3865063, @lime wrote:

which isn't clear to me what you mean

• In the function Sema::CheckTemplateArgument at line 5725, Params has been substituted in a way all TemplateTypeParmDecls are instantiated with a smaller depth, and so are constraints of them at SemaTemplateInstantiateDecl.cpp:2769, while the requires clause remains the same.
• Then CheckTemplateArgument calls CheckTemplateTemplateArgument which calls IsAtLeastAsConstrained with the original declaration and the constraints collected from Params. Thus, in IsAtLeastAsConstrained, a depth calculated from the declaration will not reflect the depth in the constraint.
• It should be fine to not adjust the depths between two constraints passed to IsAtLeastAsConstrained if the requires clause is not parsed, as they are already the same. But it is not the case when the constraint is from a requires clause, as requires clauses are not substituted. However, calculating from declarations will break the original case.

It sounds like perhaps we've instantiated constraints we shouldn't have in the case of template-template parameters. Based on what you're saying, I'm concerned then that perhaps the deferred concept instantiation didn't work right for template-template constraints? That might require more work on that then before anything could happen here.

Otherwise, I would expect calculating from the Template Template Decl to work, (though its likely it doesn't actually 'add' a layer yet, since I don't think we've needed that yet, so an extra bit of work there would need to be done).

I wouldn't expect the requires clause (nor any other concept related AST node) to be instantiated at all until it is going through 'checking'

The function TemplateDeclInstantiator::SubstTemplateParams instantiates constraints like template <template <C T> class>, so it is already happened.

If that is happening outside of a constraint evaluation, that is likely incorrect, and perhaps part of the problem.

[erichkeane]

So I've been messing around with this a bit, and am somewhat confident that IsAtLeastAsConstrainedAs should just contain:

unsigned Depth1 = CalculateTemplateDepthForConstraints(*this, D1);
unsigned Depth2 = CalculateTemplateDepthForConstraints(*this, D2);
  
for (size_t I = 0; I != AC1.size() && I != AC2.size(); ++I) {
  if (Depth2 > Depth1) {
    AC1[I] = AdjustConstraintDepth(*this, Depth2 - Depth1).
      TransformExpr(const_cast<Expr*>(AC1[I])).
      get();
  } else if (Depth1 > Depth2) {
    AC2[I] = AdjustConstraintDepth(*this, Depth1 - Depth2).
      TransformExpr(const_cast<Expr*>(AC2[I])).
      get();
  }
}

However, I see in the example:

template<typename T> concept C = T::f();
template<C> struct X{};

template<template<C> class P> struct S1 { }; // #S1
S1<X> s11;

That this fails because the type-constraint has already been instantiated during the SubstTemplateParams call ~5735 in SemaTemplate.cpp. I believe that to be an error, and the type-constraint on the TemplateTypeParmDecl should NOT be instantiated.

That line seems to change:

TemplateTypeParmDecl 0x149b2d20 <partb.cpp:4:19> col:20 Concept 0x149b2740 'C' depth 1 index 0
`-ConceptSpecializationExpr 0x149b2e58 <col:19> 'bool' Concept 0x149b2740 'C'
  `-TemplateArgument <col:20> type 'type-parameter-1-0'
    `-TemplateTypeParmType 0x149b2df0 'type-parameter-1-0' dependent depth 1 index 0
      `-TemplateTypeParm 0x149b2d20 ''

To

TemplateTypeParmDecl 0x149d1550 <partb.cpp:4:19> col:20 Concept 0x149b2740 'C' depth 0 index 0
`-ConceptSpecializationExpr 0x149d1668 <col:19> 'bool' Concept 0x149b2740 'C'
  `-TemplateArgument <col:20> type 'type-parameter-0-0'
    `-TemplateTypeParmType 0x149d15f0 'type-parameter-0-0' dependent depth 0 index 0
      `-TemplateTypeParm 0x149d1550 ''

While the TemplateTypeParmDecl is CORRECT to be changed like this, the ConceptSpecializationExpr should NOT have its arguments changed, thats the whole point of the deferred concepts implementation.

IN LOOKING, it seems that the problem is that the instantiator set up by SubstTemplateParams doesn't set the "EvaluateConstraints" to false, as it should be.

I did a test-patch here: https://reviews.llvm.org/D136468 (for you to look at, not for review!). However, I am getting 4 test failures that I suspect are a result of needing to stop evaluating constraints early elsewhere as well. I'm about done with being able to look at this today, so if you get a chance, you might find that useful to look into.

[lime]

I updated the patch based on D136468. I added a change which I think could be a fix to the specialization problem. It is just skipping the addition of the level where the specialization specialized from, as it seems to effect the calculation of the depth but not the depth in the constraint.

Denote that I got a linking error during ninja check-all, so I try to upload directly and see if it passes all the cases. And the require clauses seem not to be instantiated even if EvaluateConstraints is true.

[lime]

Update for the format check.

[erichkeane]

I'm reasonably OK with this, I'm disappointed the 'skip for specialization' is what was required, but I don't think I know of a better way. I'll hold off approving this until I've confirmed that libcxx tests + libcxx-modules-tests are properly passing as a result of this.

[erichkeane]

ALSO; this is missing the cxx_status.html and release-notes, so please re-add those!

[erichkeane]

In D134128#3879295, @erichkeane wrote:

I'm reasonably OK with this, I'm disappointed the 'skip for specialization' is what was required, but I don't think I know of a better way. I'll hold off approving this until I've confirmed that libcxx tests + libcxx-modules-tests are properly passing as a result of this.

I wnated to test this config locally, but it doesnt seem to be working for me? But you can modify this patch modify the DELETEME file in libcxx (see how he did it here: https://reviews.llvm.org/D127695 put it in the review but didnt commit it), it will cause libcxx pre-commit, which I think we shoudl do. Just don't commit the DELETEME file obviously.

[lime]

Update as required. Also, I need the following modification to complete ninja check-all. Maybe, I should submit another patch.

diff --git a/clang/unittests/Support/CMakeLists.txt b/clang/unittests/Support/CMakeLists.txt
index 956b3a7561..2413088a2b 100644
--- a/clang/unittests/Support/CMakeLists.txt
+++ b/clang/unittests/Support/CMakeLists.txt
@@ -9,4 +9,7 @@ add_clang_unittest(ClangSupportTests
 clang_target_link_libraries(ClangSupportTests
   PRIVATE
   clangFrontend
+  clangAST
+  clangSerialization
+  clangBasic
   )

[lime]

Solve the conflicts, and the issue about CMakeLists.txt had been solved by upstream.

[lime]

Update for the format check.

[lime]

Solve the format issue further.

[lime]

Solve the conflicts.

[lime]

The previous update for conflicts lacks a comma, and the comma was added.

[erichkeane]

Still need release notes!

[lime]

Update release notes as required.

[erichkeane]

I'm happy with this once the release-note is clarified and all libcxx pre-commit works with it.

clang/docs/ReleaseNotes.rst

554 ↗

(On Diff #470800)

I'm not sure what you're trying to say here? Is there just a typo that I'm missing that makes this perfectly clear?

[erichkeane]

ALSO: When you commit this: please try to use a more descriptive commit message, more similar to what I posted in Phab originally.

[lime]

Update commit message, and remove the "for" in release notes, which should make the phrase correct in grammar.

[erichkeane]

clang/docs/ReleaseNotes.rst

554 ↗

(On Diff #470800)

Sorry, I'm still getting caught up on what 'which words' means here?

[h-vetinari]

clang/docs/ReleaseNotes.rst

554 ↗

(On Diff #470800)

Maybe the intention was something along the lines of: words -> specifies

clang/test/SemaTemplate/concepts.cpp

61–62

That FIXME should be removed

[lime]

Update as suggested. Could you please help me apply this patch? It seems unsuitable to grant a fresh account the accessibility.

0001-P0857R0-Part-B-Allows-require-clauses-appearing-in-t.patch21 KBDownload

[erichkeane]

In D134128#3887549, @lime wrote:

Update as suggested. Could you please help me apply this patch? It seems unsuitable to grant a fresh account the accessibility.

0001-P0857R0-Part-B-Allows-require-clauses-appearing-in-t.patch21 KBDownload

Sure, I just need an email address and name in the form of "Name ToAppear <[e-mail]>".

[lime]

Liming Liu <[e-mail]>

It is also included in the patch file.

[erichkeane]

In D134128#3888361, @lime wrote:

Liming Liu <[e-mail]>

It is also included in the patch file.

Ok, I'll use that. Typically we don't use a separately attached patch-file, we just use the phabricator 'download patch', so I didn't think to look for it there.

[h-vetinari]

Congratulations for landing this!

How far do you (both) think we're away from completing concepts? Are the issues (aside from CWG1496 & CWG1734) mentioned on https://clang.llvm.org/cxx_status.html tracked anywhere else? I see that https://github.com/orgs/llvm/projects/14 has 45 concept-related issues, but presumably not all of those are necessary to set the feature macro?

[erichkeane]

In D134128#3890447, @h-vetinari wrote:

Congratulations for landing this!

How far do you (both) think we're away from completing concepts? Are the issues (aside from CWG1496 & CWG1734) mentioned on https://clang.llvm.org/cxx_status.html tracked anywhere else? I see that https://github.com/orgs/llvm/projects/14 has 45 concept-related issues, but presumably not all of those are necessary to set the feature macro?

There is enough on that list of failures that I'm not quite comfortable updating the macro. I think this is a question we should evaluate right before the release.

clang/lib/Parse/ParseTemplate.cpp

NamedDecl *Parser::ParseTypeParameter(unsigned Depth, unsigned Position) {
 
   return NewDecl;
 }
 
 /// ParseTemplateTemplateParameter - Handle the parsing of template
 /// template parameters.
 ///
 ///       type-parameter:    [C++ temp.param]
-///         'template' '<' template-parameter-list '>' type-parameter-key
-///                  ...[opt] identifier[opt]
-///         'template' '<' template-parameter-list '>' type-parameter-key
-///                  identifier[opt] = id-expression
+///         template-head type-parameter-key ...[opt] identifier[opt]
+///         template-head type-parameter-key identifier[opt] = id-expression
 ///       type-parameter-key:
 ///         'class'
 ///         'typename'       [C++1z]
-NamedDecl *
-Parser::ParseTemplateTemplateParameter(unsigned Depth, unsigned Position) {
+///       template-head:     [C++2a]
+///         'template' '<' template-parameter-list '>'
+///             requires-clause[opt]
+NamedDecl *Parser::ParseTemplateTemplateParameter(unsigned Depth,
+                                                  unsigned Position) {
   assert(Tok.is(tok::kw_template) && "Expected 'template' keyword");
 
   // Handle the template <...> part.
   SourceLocation TemplateLoc = ConsumeToken();
   SmallVector<NamedDecl*,8> TemplateParams;
   SourceLocation LAngleLoc, RAngleLoc;
+  ExprResult OptionalRequiresClauseConstraintER;
   {
     MultiParseScope TemplateParmScope(*this);
     if (ParseTemplateParameters(TemplateParmScope, Depth + 1, TemplateParams,
                                 LAngleLoc, RAngleLoc)) {
       return nullptr;
     }
+    if (TryConsumeToken(tok::kw_requires)) {
+      OptionalRequiresClauseConstraintER =
+          Actions.ActOnRequiresClause(ParseConstraintLogicalOrExpression(
+              /*IsTrailingRequiresClause=*/false));
+      if (!OptionalRequiresClauseConstraintER.isUsable()) {
+        SkipUntil(tok::comma, tok::greater, tok::greatergreater,
+                  StopAtSemi | StopBeforeMatch);
+        return nullptr;
+      }
+    }
   }
 
   // Provide an ExtWarn if the C++1z feature of using 'typename' here is used.
   // Generate a meaningful error if the user forgot to put class before the
   // identifier, comma, or greater. Provide a fixit if the identifier, comma,
   // or greater appear immediately or after 'struct'. In the latter case,
   // replace the keyword with 'class'.
   if (!TryConsumeToken(tok::kw_class)) {
if (Tok.is(tok::identifier)) {
     return nullptr;
   }
 
   // Recover from misplaced ellipsis.
   bool AlreadyHasEllipsis = EllipsisLoc.isValid();
   if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
     DiagnoseMisplacedEllipsis(EllipsisLoc, NameLoc, AlreadyHasEllipsis, true);
 
-  TemplateParameterList *ParamList =
-    Actions.ActOnTemplateParameterList(Depth, SourceLocation(),
-                                       TemplateLoc, LAngleLoc,
-                                       TemplateParams,
-                                       RAngleLoc, nullptr);
+  TemplateParameterList *ParamList = Actions.ActOnTemplateParameterList(
+      Depth, SourceLocation(), TemplateLoc, LAngleLoc, TemplateParams,
+      RAngleLoc, OptionalRequiresClauseConstraintER.get());
 
   // Grab a default argument (if available).
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the template parameter into the local scope.
   SourceLocation EqualLoc;
   ParsedTemplateArgument DefaultArg;
   if (TryConsumeToken(tok::equal, EqualLoc)) {
     DefaultArg = ParseTemplateTemplateArgument();

clang/lib/Sema/SemaConcept.cpp

if (!Converted.empty() && Converted[0] != nullptr)
     const_cast<FunctionDecl *>(FD)->setTrailingRequiresClause(Converted[0]);
   return false;
 }
 
 
 // Figure out the to-translation-unit depth for this function declaration for
 // the purpose of seeing if they differ by constraints. This isn't the same as
 // getTemplateDepth, because it includes already instantiated parents.
 static unsigned CalculateTemplateDepthForConstraints(Sema &S,
                                                      const NamedDecl *ND) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
       ND, nullptr, /*RelativeToPrimary*/ true,
       /*Pattern*/ nullptr,
       /*LookBeyondLambda*/ true, /*IncludeContainingStruct*/ true);
   return MLTAL.getNumSubstitutedLevels();
 }
 
 namespace {
   class AdjustConstraintDepth : public TreeTransform<AdjustConstraintDepth> {
   unsigned TemplateDepth = 0;
   public:
   using inherited = TreeTransform<AdjustConstraintDepth>;
   AdjustConstraintDepth(Sema &SemaRef, unsigned TemplateDepth)
       : inherited(SemaRef), TemplateDepth(TemplateDepth) {}
   QualType TransformTemplateTypeParmType(TypeLocBuilder &TLB,
                                          TemplateTypeParmTypeLoc TL) {
     const TemplateTypeParmType *T = TL.getTypePtr();
 
     TemplateTypeParmDecl *NewTTPDecl = nullptr;
     if (TemplateTypeParmDecl *OldTTPDecl = T->getDecl())
       NewTTPDecl = cast_or_null<TemplateTypeParmDecl>(
           TransformDecl(TL.getNameLoc(), OldTTPDecl));
 
     QualType Result = getSema().Context.getTemplateTypeParmType(
         T->getDepth() + TemplateDepth, T->getIndex(), T->isParameterPack(),
         NewTTPDecl);
     TemplateTypeParmTypeLoc NewTL = TLB.push<TemplateTypeParmTypeLoc>(Result);
     NewTL.setNameLoc(TL.getNameLoc());
     return Result;
   }
-  };
-} // namespace
 
-bool Sema::AreConstraintExpressionsEqual(const NamedDecl *Old,
-                                         const Expr *OldConstr,
-                                         const NamedDecl *New,
-                                         const Expr *NewConstr) {
+  static auto UnifyConstraintDepth(Sema &Self,
+           const NamedDecl *Old,
+           const Expr *OldConstr,
+           const NamedDecl *New,
+           const Expr *NewConstr) {
     if (Old && New && Old != New) {
-    unsigned Depth1 = CalculateTemplateDepthForConstraints(
-        *this, Old);
-    unsigned Depth2 = CalculateTemplateDepthForConstraints(
-        *this, New);
+      unsigned Depth1 = CalculateTemplateDepthForConstraints(Self, Old);
+      unsigned Depth2 = CalculateTemplateDepthForConstraints(Self, New);
 
       // Adjust the 'shallowest' verison of this to increase the depth to match
       // the 'other'.
       if (Depth2 > Depth1) {
-      OldConstr = AdjustConstraintDepth(*this, Depth2 - Depth1)
+        OldConstr = AdjustConstraintDepth(Self, Depth2 - Depth1)
                         .TransformExpr(const_cast<Expr *>(OldConstr))
                         .get();
       } else if (Depth1 > Depth2) {
-      NewConstr = AdjustConstraintDepth(*this, Depth1 - Depth2)
+        NewConstr = AdjustConstraintDepth(Self, Depth1 - Depth2)
                         .TransformExpr(const_cast<Expr *>(NewConstr))
                         .get();
       }
     }
+    return std::make_pair(OldConstr, NewConstr);
+  }
+  };
+} // namespace
 
+bool Sema::AreConstraintExpressionsEqual(const NamedDecl *Old,
+                                         const Expr *OldConstr,
+                                         const NamedDecl *New,
+                                         const Expr *NewConstr) {
+  std::tie(OldConstr, NewConstr) = AdjustConstraintDepth::UnifyConstraintDepth(
+      *this, Old, OldConstr, New, NewConstr);
   llvm::FoldingSetNodeID ID1, ID2;
   OldConstr->Profile(ID1, Context, /*Canonical=*/true);
   NewConstr->Profile(ID2, Context, /*Canonical=*/true);
   return ID1 == ID2;
 }
 
 bool Sema::FriendConstraintsDependOnEnclosingTemplate(const FunctionDecl *FD) {
   assert(FD->getFriendObjectKind() && "Must be a friend!");
 
   // The logic for non-templates is handled in ASTContext::isSameEntity, so we
   // don't have to bother checking 'DependsOnEnclosingTemplate' for a
   // non-function-template.
   assert(FD->getDescribedFunctionTemplate() &&
          "Non-function templates don't need to be checked");
 
   SmallVector<const Expr *, 3> ACs;
   FD->getDescribedFunctionTemplate()->getAssociatedConstraints(ACs);
 
   unsigned OldTemplateDepth = CalculateTemplateDepthForConstraints(*this, FD);
   for (const Expr *Constraint : ACs)
     if (ConstraintExpressionDependsOnEnclosingTemplate(OldTemplateDepth,
                                                        Constraint))
       return true;
 
   return false;
 }
static bool subsumes(Sema &S, NamedDecl *DP, ArrayRef<const Expr *> P,
   const NormalForm QCNF = makeCNF(*QNormalized);
 
   Subsumes = subsumes(PDNF, QCNF, E);
   return false;
 }
 
 bool Sema::IsAtLeastAsConstrained(NamedDecl *D1, ArrayRef<const Expr *> AC1,
                                   NamedDecl *D2, ArrayRef<const Expr *> AC2,
                                   bool &Result) {
   if (AC1.empty()) {
     Result = AC2.empty();
     return false;
   }
   if (AC2.empty()) {
     // TD1 has associated constraints and TD2 does not.
     Result = true;
     return false;
   }
 
   std::pair<NamedDecl *, NamedDecl *> Key{D1, D2};
   auto CacheEntry = SubsumptionCache.find(Key);
   if (CacheEntry != SubsumptionCache.end()) {
     Result = CacheEntry->second;
     return false;
   }
 
-  if (subsumes(*this, D1, AC1, D2, AC2, Result,
+  std::size_t I = 0;
+  SmallVector<const Expr *, 4> TAC1{AC1.size()}, TAC2{AC2.size()};
+  for (; I != AC1.size() && I != AC2.size(); ++I) {
+    std::tie(TAC1[I], TAC2[I]) = AdjustConstraintDepth::UnifyConstraintDepth(
+        *this, D1, AC1[I], D2, AC2[I]);
+  }
+  TAC1.append(AC1.begin() + I, AC1.end());
+  TAC2.append(AC2.begin() + I, AC2.end());
+
+  if (subsumes(*this, D1, TAC1, D2, TAC2, Result,
         [this] (const AtomicConstraint &A, const AtomicConstraint &B) {
           return A.subsumes(Context, B);
         }))
     return true;
   SubsumptionCache.try_emplace(Key, Result);
   return false;
 }
 

clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp

 // RUN:  %clang_cc1 -std=c++2a -frelaxed-template-template-args -verify %s
 
-template<typename T> concept C = T::f();
-// expected-note@-1{{similar constraint}}
+template<typename T> concept C = T::f(); // #C
 template<typename T> concept D = C<T> && T::g();
-template<typename T> concept F = T::f();
-// expected-note@-1{{similar constraint expressions not considered equivalent}}
-template<template<C> class P> struct S1 { }; // expected-note 2{{'P' declared here}}
+template<typename T> concept F = T::f(); // #F
+template<template<C> class P> struct S1 { }; // #S1
 
 template<C> struct X { };
-
-template<D> struct Y { }; // expected-note{{'Y' declared here}}
+template<D> struct Y { }; // #Y
 template<typename T> struct Z { };
-template<F> struct W { }; // expected-note{{'W' declared here}}
+template<F> struct W { }; // #W
 
 S1<X> s11;
-S1<Y> s12; // expected-error{{template template argument 'Y' is more constrained than template template parameter 'P'}}
+S1<Y> s12;
+// expected-error@-1 {{template template argument 'Y' is more constrained than template template parameter 'P'}}
+// expected-note@#S1 {{'P' declared here}}
+// expected-note@#Y {{'Y' declared here}}
 S1<Z> s13;
-S1<W> s14; // expected-error{{template template argument 'W' is more constrained than template template parameter 'P'}}
+S1<W> s14;
+// expected-error@-1 {{template template argument 'W' is more constrained than template template parameter 'P'}}
+// expected-note@#S1 {{'P' declared here}}
+// expected-note@#W {{'W' declared here}}
+// expected-note@#F {{similar constraint expressions not considered equivalent}}
+// expected-note@#C {{similar constraint}}
 
 template<template<typename> class P> struct S2 { };
 
 S2<X> s21;
 S2<Y> s22;
 S2<Z> s23;
 
 template <template <typename...> class C>
 struct S3;
 
 template <C T>
 using N = typename T::type;
 
 using s31 = S3<N>;
 using s32 = S3<Z>;
+
+template<template<typename T> requires C<T> class P> struct S4 { }; // #S4
+
+S4<X> s41;
+S4<Y> s42;
+// expected-error@-1 {{template template argument 'Y' is more constrained than template template parameter 'P'}}
+// expected-note@#S4 {{'P' declared here}}
+// expected-note@#Y {{'Y' declared here}}
+S4<Z> s43;
+S4<W> s44;
+// expected-error@-1 {{template template argument 'W' is more constrained than template template parameter 'P'}}
+// expected-note@#S4 {{'P' declared here}}
+// expected-note@#W {{'W' declared here}}
+// expected-note@#F {{similar constraint expressions not considered equivalent}}
+// expected-note@#C {{similar constraint}}
+
+template<template<typename T> requires C<T> typename U> struct S5 {
+  template<typename T> static U<T> V;
+};
+
+struct Nothing {};
+
+// FIXME: Wait the standard to clarify the intent.
+template<> template<> Z<Nothing> S5<Z>::V<Nothing>;

clang/test/SemaTemplate/concepts.cpp

 }
 
 namespace P0857R0 {
   void f() {
     auto x = []<bool B> requires B {}; // expected-note {{constraints not satisfied}} expected-note {{false}}
     x.operator()<true>();
     x.operator()<false>(); // expected-error {{no matching member function}}
   }
 
-  // FIXME: This is valid under P0857R0.
   template<typename T> concept C = true;
-  template<template<typename T> requires C<T> typename U> struct X {}; // expected-error {{requires 'class'}} expected-error 0+{{}}
+  template<template<typename T> requires C<T> typename U> struct X {};
   template<typename T> requires C<T> struct Y {};
-  X<Y> xy; // expected-error {{no template named 'X'}}
+  X<Y> xy;
 }
 
 namespace PR50306 {
   template<typename T> concept NotInt = sizeof(T) != sizeof(int); // expected-note {{because}}
   template<typename T> void f() {
     [](NotInt auto) {}(T()); // expected-error {{no matching function}} expected-note {{constraints not satisfied}} expected-note {{because}}
   }
   template void f<char>(); // OK

clang/www/cxx_status.html

</tr>
     </tr>
     <tr id="p0734">
       <td rowspan="13">Concepts</td>
       <td><a href="https://wg21.link/p0734r0">P0734R0</a></td>
       <td class="full" align="center">Clang 10</td>
     </tr>
       <tr> <!-- from Albuquerque -->
         <td><a href="https://wg21.link/p0857r0">P0857R0</a></td>
-        <td class="partial" align="center">
-          <details><summary>Partial</summary>
-            Constraining template template parameters is not yet supported.
-          </details>
-        </td>
+        <td class="unreleased" align="center">Clang 16</td>
       </tr>
       <tr> <!-- from San Diego -->
         <td><a href="https://wg21.link/p1084r2">P1084R2</a></td>
         <td rowspan="2" class="full" align="center">Clang 10</td>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p1141r2">P1141R2</a></td>
       </tr>