lime added a comment.
> 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?
It is explained by the inline comments in sequence.
> 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?
In the previous update, I mentioned using ArrayRef is not as efficient as
MutableArrayRef. And there was no feedback on my comment for several days, so I
changed as I said. Additionally, I found some code once called
`IsAtLeastAsConstrained` for two constraints, and then call
`IsAtLeastAsConstrained` again with the sequence of the two constraints
switched. So using MutableArrayRef also saves a potential adjustment.
Any way, adjusting depths here might unique to template template parameters. If
so, parsing the require clause in the unit test with depth equal to 0 should be
a better solution, and things about CalculateTemplateDepthForConstraints and
ArrayRef could remain unchanged.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:581
// getTemplateDepth, because it includes already instantiated parents.
static unsigned CalculateTemplateDepthForConstraints(Sema &S,
const NamedDecl *ND) {
----------------
1. The orignal `CalculateTemplateDepthForConstraints` calculates the depth from
`NamedDecl`.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:621
- if (Old && New && Old != New) {
- unsigned Depth1 = CalculateTemplateDepthForConstraints(
- *this, Old);
----------------
2. And it was only called in `AreConstraintExpressionsEqual`.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:706
+ std::tie(OldConstr, NewConstr) =
+ AdjustConstraintDepth::UnifyConstraintDepthFromDecl(*this, Old,
OldConstr,
+ New, NewConstr);
----------------
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.
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
