[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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