erichkeane marked 2 inline comments as done. erichkeane 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. WOOPS! I was wrong about 'still crashes' here. I mis-applied one of your suggestions and it crashed for another reason! When properly done, this stops that crash, and the Class-template-partial-specializations are just invalid at that point. I think we should still do the same thing as far as failing-lookup for the same reasons, so I'll do that separately. 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
