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 ();
+}
