OK, thanks.
On Wed, Sep 5, 2018 at 4:09 PM, Marek Polacek <pola...@redhat.com> wrote: > On Tue, Sep 04, 2018 at 05:29:00PM -0400, Jason Merrill wrote: >> On Tue, Sep 4, 2018 at 3:02 PM, Marek Polacek <pola...@redhat.com> wrote: >> > On Thu, Aug 30, 2018 at 09:22:26AM -0400, Jason Merrill wrote: >> >> On Wed, Aug 29, 2018 at 8:03 PM, Marek Polacek <pola...@redhat.com> wrote: >> >> > I've now gotten to the point where I question the validity of this PR, >> >> > so it's >> >> > probably a good time to stop and ask for some advice. >> >> > >> >> > As discussed in >> >> > <https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01607.html>, we >> >> > choose the wrong overload for f1: >> >> > >> >> > struct C { }; >> >> > struct A { >> >> > operator C() &; >> >> > operator C() &&; >> >> > }; >> >> > >> >> > C f1(A a) >> >> > { >> >> > return a; // should call operator C()&, but calls operator C()&& >> >> > } >> >> > >> >> > Since we're returning a local variable, we know it's about to be >> >> > destroyed, >> >> > so even though it's got a name, not for very much longer, so we >> >> > activate move >> >> > semantics. So we perform overload resolution with 'a' turned into >> >> > *NON_LVALUE_EXPR<(A&) &a>, an xvalue. We need to convert 'a' from A to >> >> > C, >> >> > which is taking place in build_user_type_conversion_1. It will see two >> >> > cadidates: >> >> > >> >> > A::operator C() && >> >> > A::operator C() & >> >> > >> >> > when adding these candidates in add_function_candidate we process the >> >> > ref-qualifiers by tweaking the type of the implicit object parameter by >> >> > turning >> >> > it into a reference type. Then we create an implicit conversion >> >> > sequence >> >> > for converting the type of the argument to the type of the parameter, >> >> > so A to A&. That succeeds in the first case (an xvalue binding to an >> >> > rvalue >> >> > reference) but fails in the second case (an xvalue binding to an lvalue >> >> > reference). And thus we end up using the first overload. >> >> > >> >> > But why is this invalid, again? [class.copy.elision] says "or if the >> >> > type of >> >> > the first parameter of the selected constructor is not an rvalue >> >> > reference to >> >> > the object's type (possibly cv-qualified), overload resolution is >> >> > performed >> >> > again, considering the object as an lvalue." but I don't see how that >> >> > applies >> >> > here. (Constructors can't have ref-qualifiers anyway.) >> >> > >> >> > Thoughts? >> >> >> >> Where that rule comes in is when we choose the constructor for C: >> >> since we've already called operator C()&&, we choose C(C&&), which >> >> does not have a first parameter of "rvalue reference to cv A", so it >> >> should be rejected. >> > >> > I see. Turned out there were two problems: we weren't getting into the >> > if (flags & LOOKUP_PREFER_RVALUE) block in build_over_call; this I fixed >> > by setting rvaluedness_matches_p in build_user_type_conversion_1 for >> > ck_rvalue >> > if appropriate. >> > >> > And then the constructor argument check failed to check the returned >> > object's >> > type. The tricky part is getting the type. I realized we can go to the >> > beginning of the conversion sequence and extract u.expr. But we can't >> > simply >> > look at its type, if it's DECL_CONV_FN_P, we need to dig deeper. You'll >> > see >> > that I don't handle other things, and I don't know if I need to. I tried >> > to >> > handle arbitrary expressions, but it evolved into something that just >> > seemed >> > too fragile. >> > >> > E.g. for this testcase u.expr will look like: >> > TARGET_EXPR <D.2335, A::operator C ((struct A *) NON_LVALUE_EXPR <(struct >> > A &) &a>)> >> > and taking its type would yield "struct C". I actually think that >> > returning >> > error_mark_node when we see any DECL_CONV_FN_P would work just as well. >> > >> > Can you think of something better than this? >> >> Hmm, I think we could push that back even farther, and bail out where >> you're already changing build_user_type_conversion_1; if we're doing >> this two-step initialization, then we aren't going to end up with a >> suitable constructor. > > That makes sense. There was one snag: we don't want to make a candidate > non-viable only because of LOOKUP_PREFER_RVALUE before the checking if > the conversion was ambiguous: if we have two candidates, then killing one > of them would make the conversion non-ambiguous (as in template/spec22.C). > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-09-05 Marek Polacek <pola...@redhat.com> > > PR c++/87109, wrong overload with ref-qualifiers. > * call.c (build_user_type_conversion_1): Use NULL instead of 0. Bail > out if performing the maybe-rvalue overload resolution and a > conversion > function is getting called. > > * g++.dg/cpp0x/ref-qual19.C: New test. > > diff --git gcc/cp/call.c gcc/cp/call.c > index a1567026975..942b2c204be 100644 > --- gcc/cp/call.c > +++ gcc/cp/call.c > @@ -3971,7 +3971,7 @@ build_user_type_conversion_1 (tree totype, tree expr, > int flags, > } > > cand = tourney (candidates, complain); > - if (cand == 0) > + if (cand == NULL) > { > if (complain & tf_error) > { > @@ -4014,6 +4014,12 @@ build_user_type_conversion_1 (tree totype, tree expr, > int flags, > if (cand->viable == -1) > conv->bad_p = true; > > + /* We're performing the maybe-rvalue overload resolution and > + a conversion function is in play. This isn't going to work > + because we would not end up with a suitable constructor. */ > + if ((flags & LOOKUP_PREFER_RVALUE) && !DECL_CONSTRUCTOR_P (cand->fn)) > + return NULL; > + > /* Remember that this was a list-initialization. */ > if (flags & LOOKUP_NO_NARROWING) > conv->check_narrowing = true; > diff --git gcc/testsuite/g++.dg/cpp0x/ref-qual19.C > gcc/testsuite/g++.dg/cpp0x/ref-qual19.C > index e69de29bb2d..8494b83e5b0 100644 > --- gcc/testsuite/g++.dg/cpp0x/ref-qual19.C > +++ gcc/testsuite/g++.dg/cpp0x/ref-qual19.C > @@ -0,0 +1,117 @@ > +// PR c++/87109 > +// { dg-do run { target c++11 } } > + > +#include <utility> > + > +struct C { int i; }; > +struct D { int i; }; > + > +struct A { > + int j; > + operator C() & { return { 1 }; } > + operator C() && { return { 2 }; } > +}; > + > +struct B : public A { > + operator D() & { return { 3 }; } > + operator D() && { return { 4 }; } > +}; > + > +C > +f (A a) > +{ > + return a; > +} > + > +C > +f2 (A a) > +{ > + return std::move (a); > +} > + > +C > +f3 () > +{ > + A a; > + return a; > +} > + > +C > +f4 () > +{ > + A a; > + return std::move (a); > +} > + > +C > +f5 () > +{ > + return A(); > +} > + > +D > +f6 (B b) > +{ > + return b; > +} > + > +D > +f7 (B b) > +{ > + return std::move (b); > +} > + > +D > +f8 () > +{ > + B b; > + return b; > +} > + > +D > +f9 () > +{ > + B b; > + return std::move (b); > +} > + > +D > +f10 () > +{ > + return B(); > +} > + > +int > +main () > +{ > + C c1 = f (A()); > + if (c1.i != 1) > + __builtin_abort (); > + C c2 = f2 (A()); > + if (c2.i != 2) > + __builtin_abort (); > + C c3 = f3 (); > + if (c3.i != 1) > + __builtin_abort (); > + C c4 = f4 (); > + if (c4.i != 2) > + __builtin_abort (); > + C c5 = f5 (); > + if (c5.i != 2) > + __builtin_abort (); > + D c6 = f6 (B()); > + if (c6.i != 3) > + __builtin_abort (); > + D c7 = f7 (B()); > + if (c7.i != 4) > + __builtin_abort (); > + D c8 = f8 (); > + if (c8.i != 3) > + __builtin_abort (); > + D c9 = f9 (); > + if (c9.i != 4) > + __builtin_abort (); > + D c10 = f10 (); > + if (c10.i != 4) > + __builtin_abort (); > +}
