Re: C++ PATCH to refine c++/87109 patch

[Marek Polacek <polacek at redhat dot com>]

On Thu, Sep 20, 2018 at 12:20:08PM -0400, Jason Merrill wrote:
> On Thu, Sep 20, 2018 at 11:53 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Thu, Sep 20, 2018 at 11:25:38AM -0400, Jason Merrill wrote:
> >> On Wed, Sep 19, 2018 at 9:50 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> > Aaaand this addresses <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87150#c11>,
> >> > as I promised earlier.  I hope I got it right.
> >> >
> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >> >
> >> > 2018-09-19  Marek Polacek  <polacek@redhat.com>
> >> >
> >> >         PR c++/87109 - wrong ctor with maybe-rvalue semantics.
> >> >         * call.c (build_user_type_conversion_1): Refine the maybe-rvalue
> >> >         check to only return if we're converting from a base class.
> >> >
> >> >         * g++.dg/cpp0x/ref-qual19.C: Adjust the expected results.
> >> >         * g++.dg/cpp0x/ref-qual20.C: New test.
> >> >
> >> > diff --git gcc/cp/call.c gcc/cp/call.c
> >> > index ddf0ed044a0..4bbd77b9cef 100644
> >> > --- gcc/cp/call.c
> >> > +++ gcc/cp/call.c
> >> > @@ -4034,9 +4034,13 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
> >> >      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))
> >> > +     a conversion function is in play.  If we're converting from
> >> > +     a base class to a derived class, reject the conversion.  */
> >> > +  if ((flags & LOOKUP_PREFER_RVALUE)
> >> > +      && !DECL_CONSTRUCTOR_P (cand->fn)
> >> > +      && CLASS_TYPE_P (fromtype)
> >> > +      && CLASS_TYPE_P (totype)
> >> > +      && DERIVED_FROM_P (fromtype, totype))
> >>
> >> Here fromtype is the type we're converting from, and what we want to
> >> reject is converting the return value of the conversion op to a base
> >> class.  CLASS_TYPE_P (fromtype) will always be true, since it has a
> >> conversion op.  And I think we also want to handle the case of totype
> >> being a reference.
> >
> > I think I totally misunderstood what this was about.  It's actually about
> > this case
> >
> > struct Y { int y; };
> > struct X : public Y { int x; };
> >
> > struct A {
> >   operator X();
> > };
> >
> > Y
> > fn (A a)
> > {
> >   return a;
> > }
> >
> > where we want to avoid slicing of X when converting X to Y, yes?
> 
> Yes.

Got it.  So I think the following is the real fix then:

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2018-09-20  Marek Polacek  <polacek@redhat.com>

	PR c++/87109 - wrong ctor with maybe-rvalue semantics.
	* call.c (build_user_type_conversion_1): Refine the maybe-rvalue
	check to only return if we're converting the return value to a base
	class.

	* g++.dg/cpp0x/ref-qual19.C: Adjust the expected results.
	* g++.dg/cpp0x/ref-qual20.C: New test.

diff --git gcc/cp/call.c gcc/cp/call.c
index ddf0ed044a0..b2ca667c8b4 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -4034,10 +4034,12 @@ build_user_type_conversion_1 (tree totype, tree expr, int flags,
     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.  */
+     a conversion function is in play.  Reject converting the return
+     value of the conversion function to a base class.  */
   if ((flags & LOOKUP_PREFER_RVALUE) && !DECL_CONSTRUCTOR_P (cand->fn))
-    return NULL;
+    for (conversion *t = cand->second_conv; t; t = next_conversion (t))
+      if (t->kind == ck_base)
+	return NULL;
 
   /* Remember that this was a list-initialization.  */
   if (flags & LOOKUP_NO_NARROWING)
diff --git gcc/testsuite/g++.dg/cpp0x/ref-qual19.C gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
index 8494b83e5b0..50f92977c49 100644
--- gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
+++ gcc/testsuite/g++.dg/cpp0x/ref-qual19.C
@@ -85,13 +85,13 @@ int
 main ()
 {
   C c1 = f (A());
-  if (c1.i != 1)
+  if (c1.i != 2)
     __builtin_abort ();
   C c2 = f2 (A());
   if (c2.i != 2)
     __builtin_abort ();
   C c3 = f3 ();
-  if (c3.i != 1)
+  if (c3.i != 2)
     __builtin_abort ();
   C c4 = f4 ();
   if (c4.i != 2)
@@ -100,13 +100,13 @@ main ()
   if (c5.i != 2)
     __builtin_abort ();
   D c6 = f6 (B());
-  if (c6.i != 3)
+  if (c6.i != 4)
     __builtin_abort ();
   D c7 = f7 (B());
   if (c7.i != 4)
     __builtin_abort ();
   D c8 = f8 ();
-  if (c8.i != 3)
+  if (c8.i != 4)
     __builtin_abort ();
   D c9 = f9 ();
   if (c9.i != 4)
diff --git gcc/testsuite/g++.dg/cpp0x/ref-qual20.C gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
index e69de29bb2d..c8bd43643af 100644
--- gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
+++ gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
@@ -0,0 +1,70 @@
+// PR c++/87109
+// { dg-do run { target c++11 } }
+
+#include <utility>
+
+struct Y {
+  int y;
+  Y(int y_) : y(y_) { }
+};
+struct X : public Y {
+  int x;
+  X(int x_, int y_) : x(x_), Y(y_) { }
+};
+
+struct A {
+  operator X() & { return { 0, 2 }; }
+  operator X() && { return { 0, -1 }; }
+};
+
+Y
+f (A a)
+{
+  return a;
+}
+
+Y
+f2 (A a)
+{
+  return std::move (a);
+}
+
+Y
+f3 ()
+{
+  A a;
+  return a;
+}
+
+Y
+f4 ()
+{
+  A a;
+  return std::move (a);
+}
+
+Y
+f5 ()
+{
+  return A();
+}
+
+int
+main ()
+{
+  Y y1 = f (A());
+  if (y1.y != 2)
+    __builtin_abort ();
+  Y y2 = f2 (A());
+  if (y2.y != -1)
+    __builtin_abort ();
+  Y y3 = f3 ();
+  if (y3.y != 2)
+    __builtin_abort ();
+  Y y4 = f4 ();
+  if (y4.y != -1)
+    __builtin_abort ();
+  Y y5 = f5 ();
+  if (y5.y != -1)
+    __builtin_abort ();
+}