rsmith added a comment.
This all looks good to me. Sorry for the delay.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:728
bool SkipForSpecialization = false) {
MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
ND, /*Final=*/false, /*Innermost=*/nullptr, /*RelativeToPrimary=*/true,
----------------
erichkeane wrote:
> Note all uses of THIS function are probably wrong based on what Richard says?
> I think even the friend-constraint-depends-on-enclosing-template thing is
> wrong? Is that right @rsmith ?
I've looked through them and yeah, I think they should all be replaced by
something else -- though that seems less urgent than this change. One important
goal should be removal of `AdjustConstraintDepth` and
`ConstraintRefersToContainingTemplateChecker` -- a `TreeTransform` subclass is
*hugely* expensive in terms of code size, and we don't need one here, let alone
two.
Specifically:
- `AreConstraintExpressionsEqual` we've already discussed in this PR; this code
is wrong.
- In `FriendConstraintDependOnEnclosingTemplate`, the depth calculation seems
fine, but `ConstraintExpressionDependsOnEnclosingTemplate` uses a
`TreeTransform` just to see if the original expression mentioned any enclosing
template parameters, which is really inefficient and expensive. We should
change that to use `Sema::MarkUsedTemplateParameters` instead. It has a
`RecursiveASTVisitor` that computes this. It would need some changes to add a
mode to detect template parameters at <= depth instead of at == depth, or we
could call it in a loop I suppose.
- In `IsAtLeastAsConstrained`, it's really not clear what the intended language
behavior is when the constraints have different template depths. I think there
are two interesting cases here:
- The case where one or both functions are friends.
- The case where one function is a member and the other is a non-member,
during operator lookup.
It's not really obvious what the right behavior is in either case. What we're
currently doing doesn't make a lot of sense -- unrelated enclosing template
parameters from different templates will get compared and considered equivalent
-- but I don't have much of a better answer. Perhaps the thing that makes the
most sense is to perform a substitution (`SubstExpr`) for non-member-like
constrained friends (see https://reviews.llvm.org/D147068 and
https://eel.is/c++draft/temp.friend#9 for context) and consider any other case
where the functions were declared in different template scopes to have no
ordering by constraints.
I do wonder whether, in the case of a non-member-like constrained friend
function template, we should actually do the depth adjustment as part of
instantiation (that is, substitute into the constraint in that case). That
would make the rest of the handling simpler: no depth adjustment would be
necessary in `IsAtLeastAsConstrained`.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+ OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+ SourceLocation(), false /* PartialOrdering */);
bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),
----------------
shafik wrote:
> nit
Just remove the final parameter; it has a default argument of `false` and no
other call site passes `false` here. (I'm working on removing this parameter
in a different change.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146178/new/
https://reviews.llvm.org/D146178
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
