ychen added a comment. In D134542#3812292 <https://reviews.llvm.org/D134542#3812292>, @erichkeane wrote:
> 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. Agreed that short-circuiting cases would not hit this issue. I actually meant cases where a supposedly failed constraint has errors (https://clang.godbolt.org/z/5P7fYYvao). But that could be reconsidered in the future if use cases arise. With this patch, we already handle this better than GCC/MSVC. constexpr bool True = true; constexpr bool False = b; template <typename T> concept C = True; template <typename T> concept D = False; template<D T> void foo(T) = delete; template<C T> void foo(T); void g() { foo(1); } ================ Comment at: clang/include/clang/Sema/Overload.h:931 + bool ConstraintFailureBecauseCascadingError() const; + ---------------- erichkeane wrote: > 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! Thanks. This looks good to me. 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
