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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
