[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:590
 
+// FIXME: may be incomplete
+static unsigned CalculateTemplateDepthForConstraints(const Expr *Constr) {
----------------
lime wrote:
> erichkeane wrote:
> > 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.
> 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.
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.


================
Comment at: clang/lib/Sema/SemaConcept.cpp:706
+  std::tie(OldConstr, NewConstr) =
+      AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old, OldConstr,
+                                                          New, NewConstr);
----------------
lime wrote:
> 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.
Right, but I'm saying how you're doing it is incorrect and will likely result in incorrect compilations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134128