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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
