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