[PATCH] D45680: [C++2a] Add operator<=> Rewriting - Early Attempt

[Eric Fiselier via Phabricator via cfe-commits]

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