EricWF marked 2 inline comments as done. EricWF added inline comments.
================ Comment at: lib/Sema/SemaOverload.cpp:12537-12565 +/// Rewritten candidates have been added but not checked for validity. They +/// could still be non-viable if: +/// (A) The rewritten call (x <=> y) is a builtin, but it will be ill-formed +/// when built (for example it has narrowing conversions). +/// (B) The expression (x <=> y) @ 0 is ill-formed for the result of (x <=> y). +/// +/// If either is the case, this function should be considered non-viable and ---------------- EricWF wrote: > rsmith wrote: > > Hmm, this is an interesting approach. It's quite a divergence from our > > usual approach, which is to perform the viability checks first, before > > partial ordering, even if we could eliminate some candidates based on > > partial ordering. And it's observable -- it will result in fewer template > > instantiations being performed, which will mean we do not diagnose some > > ill-formed (no diagnostic required) situations which would otherwise fail > > due to an error in a non-immediate context. > > > > Can you say something about why you chose this approach instead of doing > > the lookup for operator `@` early and marking the candidate non-viable if > > the lookup / overload resolution for it fails? > > > > (I'm also vaguely wondering whether we should be pushing back on the C++ > > committee for making viability of the rewritten candidate depend on > > validity of the `@` expression, not just the `<=>` expression.) > The main reason for choosing this approach was the current expense of > validating the candidates eagerly when they're added. Currently builtin > candidates need to be fully built before their viability is known. My initial > attempt at this made the compiler just about hang. > > If I understand what you mean by performing lookup/overload resolution early > for operator `@`, and I probably don't, then it would require performing > overload resolution separately every rewritten candidate added, since each > candidate could have a different return type. > > I think rewritten builtins could be checked more eagerly by moving a bunch of > the checking from `SemaExpr.cpp` to `SemaOverload.cpp`, and using it to > eagerly check the bits we need to, but not actually building the expression > as calling into `SemaExpr.cpp` would currently do. > > As for the root question: Should the validity of the `@` expression be > considered during overload resolution? I think the answer is tricky. For > example, the following case could easily occur and break existing code, > especially as more users add defaulted comparison operators. In this case if > the base class adds <=> without the derived classes knowledge. > > ``` > struct B { > using CallbackTy = void(*)(); > CallbackTy CB = nullptr; // B carries around a function pointer. > #if __cplusplus >= 201703L > friend auto operator<=>(B const&, B const&) = default; > #else > friend bool operator==(B, B); > friend bool operator!=(B, B); > #endif > }; > struct D { > operator int() const; // D uses this conversion to provide comparisons. > }; > D d; > // comparison ill-formed if the `@` expression isn't considered for viability. > auto r = d < d; > ``` > However, allowing the `@` expression to effect viability leads to a different > set of bugs. For example this slicing comparison bug: > ``` > struct B { > int x; > friend auto operator<=>(B, B) = default; > }; > struct D : B { > decltype(nullptr) y = {}; > friend auto operator<=>(D, D) = default; > }; > D d; > // If the `@` expression is considered for viability, then `operator<=>(B, > B)` will be used, effectively slicing the data used during the comparison. > auto r = d < d; > ``` > > What is clear is that it should go back to CWG for more discussion. > > It's quite a divergence from our usual approach, which is to perform the > viability checks first, before partial ordering, even if we could eliminate > some candidates based on partial ordering. And it's observable -- it will > result in fewer template instantiations being performed, which will mean we > do not diagnose some ill-formed (no diagnostic required) situations which > would otherwise fail due to an error in a non-immediate context. I should clarify that the same validity checks are applied to rewritten candidates are also applied to non-rewritten candidates, such as checking conversions to parameter types. So I suspect, for all intents and purposes, it should produce the same amount of template instantiations as existing operator lookup does. Repository: rC Clang https://reviews.llvm.org/D45680 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits