erichkeane marked 3 inline comments as done. erichkeane added a subscriber: tahonermann. erichkeane added a comment.
In D134542#3812211 <https://reviews.llvm.org/D134542#3812211>, @ychen wrote: > The patch looks good. Two high-level questions: > > - Does the similar thing happen for class templates? Like a constraint expr > of a partial specialization has an error. Maybe we don't assert in this case? WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 . This patch still crashes in that case, and we should fix that in a similar way. I'll put it on my list of things to do soon! I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch. > - I suppose the constraint error does not always affect the overload > resolution results. When it does not, an alternative would be to assume the > constraint is a satisfaction failure and the compilation continues. Do you > see any value in this approach? Personally, I could go either way. Basically > a trade-off between pessimistic and optimistic. In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it. ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution. I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors. Consider something like: template<typename T> concept HasFoo = /*Test for has foo*/; template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/; template<typename T> requires HasFoo<T> void do_thing(T &t) { t.Foo(); t.Bar(); } template<typename T> requires HasFoo<T> && HasBarAlternative<T> void do_thing(T&t) { t.Foo(); t.BarAlternative(); } The result of just marking `HasBarAlternative' as not satisfied, is the 1st `do_thing` will be called. THEN you'd get an error on instantiating because of the lack of `Bar`. This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time. ================ Comment at: clang/include/clang/Sema/Overload.h:931 + bool ConstraintFailureBecauseCascadingError() const; + ---------------- ychen wrote: > How about `ConstraintExprHasError`? `Cascading` sounds like more details than > useful. Yeah, my name is awful here... I'm not sure `ConstraintExprHasError` is correc tin this case (since this is an OverloadCandidate), so the question is "Does this candidate fail because this is a constraint that contains an error". I'll try to come up with something better. Feel free to help bikeshed/workshop something better! ================ Comment at: clang/lib/Sema/SemaOverload.cpp:10123-10127 + if (DeductionFailure.Result != Sema::TDK_ConstraintsNotSatisfied) + return false; + + return static_cast<CNSInfo *>(DeductionFailure.Data) + ->Satisfaction.ContainsErrors; ---------------- ychen wrote: > nit: might be easier to read yep! I like it. I tend to do early exit when I can, particularly on large functions, but this ended up being pretty small in the final version of this patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134542/new/ https://reviews.llvm.org/D134542 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
