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

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a comment.

Hmm... there is a lot going on here that I'm not sure is necessary? I was able to change the `AreConstraintExpressionsEqual` 'if' body to just:

  +    MultiLevelTemplateArgumentList OldMLTAL =
  +        getTemplateArgumentsForComparison(*this, Old);
  +    MultiLevelTemplateArgumentList NewMLTAL =
  +        getTemplateArgumentsForComparison(*this, New); // just does what you did to get the MLTAL, but in a separate function.
  +
  +    ExprResult OldConstrRes = SubstConstraintExpr(OldConstr, OldMLTAL);
  +    ExprResult NewConstrRes = SubstConstraintExpr(NewConstr, NewMLTAL);
  +
  +    if (!OldConstrRes.isUsable() || !NewConstrRes.isUsable())
  +      return false;
  +
  +    OldConstr = OldConstrRes.get();
  +    NewConstr = NewConstrRes.get();

and the changes to SemaTemplateInstantiateDecl don't seem necessary?  Though I see the SFINAE trap is perhaps a good idea.

With that all done, the it seemed the only problem was with how a requires clause was instantiated(which I suspect needs to just not check the constraint always). But I don't see the rest of this patch is relevant?  I DO note that with the change above, your test all works.



================
Comment at: clang/lib/Sema/SemaConcept.cpp:744
 namespace {
   class AdjustConstraintDepth : public TreeTransform<AdjustConstraintDepth> {
   unsigned TemplateDepth = 0;
----------------
This class here should probably go away now, since we're replacing it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146178/new/

https://reviews.llvm.org/D146178