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

[Liming Liu via Phabricator via cfe-commits]

lime added a comment.

> 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 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 switched. So using MutableArrayRef also saves a potential adjustment.
Any way, adjusting depths here might 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.



================
Comment at: clang/lib/Sema/SemaConcept.cpp:581
 // getTemplateDepth, because it includes already instantiated parents.
 static unsigned CalculateTemplateDepthForConstraints(Sema &S,
                                                      const NamedDecl *ND) {
----------------
1. The orignal `CalculateTemplateDepthForConstraints` calculates the depth from `NamedDecl`.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:621
-  if (Old && New && Old != New) {
-    unsigned Depth1 = CalculateTemplateDepthForConstraints(
-        *this, Old);
----------------
2. And it was only called in `AreConstraintExpressionsEqual`.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:706
+  std::tie(OldConstr, NewConstr) =
+      AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old, OldConstr,
+                                                          New, NewConstr);
----------------
erichkeane wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134128/new/

https://reviews.llvm.org/D134128