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?
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2018-09-04 Marek Polacek <pola...@redhat.com>
PR c++/87109
* call.c (build_user_type_conversion_1): If LOOKUP_PREFER_RVALUE, set
rvaluedness_matches_p on ck_rvalue.
(source_expr): New.
(build_over_call): Check if the returned object's type is the same as
the argument this ctor received.
* g++.dg/cpp0x/ref-qual19.C: New test.
diff --git gcc/cp/call.c gcc/cp/call.c
index a1567026975..6f998e1201a 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -3919,7 +3919,11 @@ build_user_type_conversion_1 (tree totype, tree expr,
int flags,
harmless since we'd add it here anyway. */
if (ics && MAYBE_CLASS_TYPE_P (totype) && ics->kind != ck_rvalue
&& !(convflags & LOOKUP_NO_TEMP_BIND))
- ics = build_conv (ck_rvalue, totype, ics);
+ {
+ ics = build_conv (ck_rvalue, totype, ics);
+ if (flags & LOOKUP_PREFER_RVALUE)
+ ics->rvaluedness_matches_p = true;
+ }
cand->second_conv = ics;
@@ -7722,6 +7726,16 @@ build_trivial_dtor_call (tree instance)
instance, clobber);
}
+/* The source expr for this standard conversion sequence. */
+
+static tree
+source_expr (conversion *t)
+{
+ while (t->kind != ck_identity)
+ t = next_conversion (t);
+ return t->u.expr;
+}
+
/* Subroutine of the various build_*_call functions. Overload resolution
has chosen a winning candidate CAND; build up a CALL_EXPR accordingly.
ARGS is a TREE_LIST of the unconverted arguments to the call. FLAGS is a
@@ -7934,6 +7948,25 @@ build_over_call (struct z_candidate *cand, int flags,
tsubst_flags_t complain)
|| !TYPE_REF_IS_RVALUE (ptype)
|| CONVERSION_RANK (convs[0]) > cr_exact)
return error_mark_node;
+
+ ptype = non_reference (convs[0]->type);
+
+ /* Find what we're converting from. */
+ tree t = source_expr (convs[0]);
+ if (TREE_CODE (t) == TARGET_EXPR)
+ t = TARGET_EXPR_INITIAL (t);
+ tree fndecl = cp_get_callee_fndecl_nofold (t);
+ /* We know now that the ctor takes an rvalue reference, now
+ check that its object has the same as the returned object's
+ type. This assumes that the types may differ only if a
+ user-defined conversion is in play. */
+ if (fndecl && DECL_CONV_FN_P (fndecl))
+ {
+ t = DECL_ARGUMENTS (fndecl);
+ t = TREE_TYPE (TREE_TYPE (t));
+ if (!same_type_ignoring_top_level_qualifiers_p (t, ptype))
+ return error_mark_node;
+ }
}
}
/* Bypass access control for 'this' parameter. */
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 ();
+}
