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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


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


================
Comment at: clang/lib/Sema/SemaConcept.cpp:1340
+bool Sema::IsAtLeastAsConstrained(NamedDecl *D1,
+                                  MutableArrayRef<const Expr *> AC1,
+                                  NamedDecl *D2,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128