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

[Liming Liu via Phabricator via cfe-commits]

lime added inline comments.


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


================
Comment at: clang/lib/Sema/SemaConcept.cpp:1340
+bool Sema::IsAtLeastAsConstrained(NamedDecl *D1,
+                                  MutableArrayRef<const Expr *> AC1,
+                                  NamedDecl *D2,
----------------
erichkeane wrote:
> erichkeane wrote:
> > 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 
> 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.
The function `MaybeEmitAmbiguousAtomicConstraintDiagnostic` calls `subsume` on `AtomicConstraint` without adjusting depths. So, I guess it is fine to reuse the unified arrays.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128