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

Reply via email to