On 10/6/22 17:43, Marek Polacek wrote:
On Thu, Oct 06, 2022 at 02:00:40PM -0400, Jason Merrill wrote:
On 10/6/22 13:51, Marek Polacek wrote:
On Thu, Oct 06, 2022 at 10:58:44AM -0400, Jason Merrill wrote:
On 10/6/22 10:49, Marek Polacek wrote:
On Wed, Oct 05, 2022 at 08:25:29PM -0400, Jason Merrill wrote:
On 10/5/22 17:27, Marek Polacek wrote:
This PR reports that

      struct Base {};
      struct Derived : Base {};
      static_assert(__reference_constructs_from_temporary(Base const&, 
Derived));

doesn't pass, which it should: it's just like

      const Base& b(Derived{});

where we bind 'b' to the Base subobject of a temporary object of type
Derived.  The ck_base conversion didn't have ->need_temporary_p set because
we didn't need to create a temporary object just for the base, but the whole
object is a temporary so we're still binding to a temporary.  Fixed by
the conv_is_prvalue hunk.

That broke a bunch of tests.  I've distilled the issue into a simple test
in elision4.C.  Essentially, we have

      struct B { /* ... */ };
      struct D : B { };
      B b = D();

and we set force_elide in build_over_call, but we're unable to actually
elide the B::B(B&&) call, and crash on gcc_assert (!force_elide);.

<https://en.cppreference.com/w/cpp/language/copy_elision> says that copy
elision "can only apply when the object being initialized is known not to be
a potentially-overlapping subobject".  So I suppose we shouldn't force_elide
the B::B(B&&) call.  I don't belive the CWG 2327 code was added to handle
derived-to-base conversions, at that time conv_binds_ref_to_prvalue wasn't
checking ck_base at all.

Does that make sense?  If so...

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

        PR c++/107085

gcc/cp/ChangeLog:

        * call.cc (conv_is_prvalue): Return true if the base subobject is part
        of a temporary object.

No, the base subobject of a prvalue is an xvalue.

Ah, so this is just like T().m where T() is a prvalue but the whole thing
is an xvalue.  Duly noted.

Exactly.

I think the problem is that an expression being a prvalue is a subset of
binding a reference to a temporary, and we shouldn't try to express both of
those using the same function: you need a separate
conv_binds_ref_to_temporary.

Ack, so how about this?  Thanks,

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

-- >8 --
This PR reports that

     struct Base {};
     struct Derived : Base {};
     static_assert(__reference_constructs_from_temporary(Base const&, Derived));

doesn't pass, which it should: it's just like

     const Base& b(Derived{});

where we bind 'b' to the Base subobject of a temporary object of type
Derived.  The ck_base conversion didn't have ->need_temporary_p set because
we didn't need to create a temporary object just for the base, but the whole
object is a temporary so we're still binding to a temporary.  Since the
Base subobject is an xvalue, a new function is introduced.

        PR c++/107085

gcc/cp/ChangeLog:

        * call.cc (conv_binds_ref_to_temporary): New.
        (ref_conv_binds_directly): Use it.

gcc/testsuite/ChangeLog:

        * g++.dg/ext/reference_constructs_from_temporary1.C: Adjust expected
        result.
        * g++.dg/ext/reference_converts_from_temporary1.C: Likewise.
        * g++.dg/cpp0x/elision4.C: New test.
---
    gcc/cp/call.cc                                | 23 ++++++++++++++++++-
    gcc/testsuite/g++.dg/cpp0x/elision4.C         | 15 ++++++++++++
    .../reference_constructs_from_temporary1.C    |  2 +-
    .../ext/reference_converts_from_temporary1.C  |  2 +-
    4 files changed, 39 insertions(+), 3 deletions(-)
    create mode 100644 gcc/testsuite/g++.dg/cpp0x/elision4.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index bd04a1d309a..715a83f5a69 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -9210,6 +9210,27 @@ conv_binds_ref_to_prvalue (conversion *c)
      return conv_is_prvalue (next_conversion (c));
    }
+/* True iff C is a conversion that binds a reference to a temporary.
+   This is a superset of conv_binds_ref_to_prvalue: here we're also
+   interested in xvalues.  */
+
+static bool
+conv_binds_ref_to_temporary (conversion *c)
+{
+  if (conv_binds_ref_to_prvalue (c))
+    return true;
+  if (c->kind != ck_ref_bind)
+    return false;
+  c = next_conversion (c);
+  /* This is the case for
+       struct Base {};
+       struct Derived : Base {};
+       const Base& b(Derived{});
+     where we bind 'b' to the Base subobject of a temporary object of type
+     Derived.  The subobject is an xvalue; the whole object is a prvalue.  */
+  return (c->kind == ck_base && conv_is_prvalue (next_conversion (c)));

I think you also want to check for the case of c->u.expr being a
COMPONENT_REF/ARRAY_REF around a TARGET_EXPR, as you mentioned.

I see.  So this would be achieved using e.g.

struct B { };
struct D : B { };
struct C {
    D d;
};

const B& b = C{}.d;

Yes.

Except I'm not sure how to trigger this via the built-in, which takes two types.
Am I missing something obvious?

Indeed, it can't be triggered by the built-in.  But I see
ref_conv_binds_directly is also called from warn_for_range_copy, which ought
to be able to trigger it.

Even there, I think, it's not possible to trigger it with, say (d is an
array of D):

      for (const B &b : C{}.d)
        // ...

because warn_for_range_copy gets *__for_begin as the EXPR, which means that
we won't get to the TARGET_EXPR.  It's like

   auto&& __for_begin = C{}.d;
   const B &b = *__for_begin;

and the conversion warn_for_range_copy sees is D -> const B -> const B&, the
original .u.expr is *__for_begin.

Ah, right, and so the C{} temporary gets lifetime-extended and there's no problem to warn about.

I could add some checking assert to conv_binds_ref_to_temporary to see if
we ever encounter a COMPONENT_REF/ARRAY_REF around a TARGET_EXPR...

Or try to handle it properly even if we can't exercise the code yet.

I'm thinking again about warning for e.g.

const B& b = frotz(C{}.d);

where frotz passes through its reference argument, which becomes a dangling reference. Handling the C{}.d case seems relevant to such a warning.

I wonder how many false positives there would be for warning about this for any function that both takes and returns a reference?

Do you want to try that, or shall I?

Incidentally, ref_conv_binds_directly should also probably be reversed to
ref_conv_binds_to_temporary since you can "bind directly" to an xvalue that
refers to a temporary.

Good point, I at least did that.

-- >8 --
This PR reports that

   struct Base {};
   struct Derived : Base {};
   static_assert(__reference_constructs_from_temporary(Base const&, Derived));

doesn't pass, which it should: it's just like

   const Base& b(Derived{});

where we bind 'b' to the Base subobject of a temporary object of type
Derived.  The ck_base conversion didn't have ->need_temporary_p set because
we didn't need to create a temporary object just for the base, but the whole
object is a temporary so we're still binding to a temporary.  Since the
Base subobject is an xvalue, a new function is introduced.

        PR c++/107085

gcc/cp/ChangeLog:

        * call.cc (conv_binds_ref_to_temporary): New.
        (ref_conv_binds_directly): Rename to...
        (ref_conv_binds_to_temporary): ...this.  Use
        conv_binds_ref_to_temporary.
        * cp-tree.h (ref_conv_binds_directly): Rename to...
        (ref_conv_binds_to_temporary): ...this.
        * method.cc (ref_xes_from_temporary): Use ref_conv_binds_to_temporary.
        * parser.cc (warn_for_range_copy): Likewise.

gcc/testsuite/ChangeLog:

        * g++.dg/ext/reference_constructs_from_temporary1.C: Adjust expected
        result.
        * g++.dg/ext/reference_converts_from_temporary1.C: Likewise.
        * g++.dg/cpp0x/elision4.C: New test.
---
  gcc/cp/call.cc                                | 33 +++++++++++++++----
  gcc/cp/cp-tree.h                              |  2 +-
  gcc/cp/method.cc                              |  2 +-
  gcc/cp/parser.cc                              |  5 +--
  gcc/testsuite/g++.dg/cpp0x/elision4.C         | 15 +++++++++
  .../reference_constructs_from_temporary1.C    |  2 +-
  .../ext/reference_converts_from_temporary1.C  |  2 +-
  7 files changed, 49 insertions(+), 12 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/cpp0x/elision4.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index bd04a1d309a..5a3d509f6c8 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -9210,15 +9210,36 @@ conv_binds_ref_to_prvalue (conversion *c)
    return conv_is_prvalue (next_conversion (c));
  }
-/* Return tristate::TS_TRUE if converting EXPR to a reference type TYPE does
-   not involve creating a temporary.  Return tristate::TS_FALSE if converting
-   EXPR to a reference type TYPE binds the reference to a temporary.  If the
-   conversion is invalid or bad, return tristate::TS_UNKNOWN.  DIRECT_INIT_P
+/* True iff C is a conversion that binds a reference to a temporary.
+   This is a superset of conv_binds_ref_to_prvalue: here we're also
+   interested in xvalues.  */
+
+static bool
+conv_binds_ref_to_temporary (conversion *c)
+{
+  if (conv_binds_ref_to_prvalue (c))
+    return true;
+  if (c->kind != ck_ref_bind)
+    return false;
+  c = next_conversion (c);
+  /* This is the case for
+       struct Base {};
+       struct Derived : Base {};
+       const Base& b(Derived{});
+     where we bind 'b' to the Base subobject of a temporary object of type
+     Derived.  The subobject is an xvalue; the whole object is a prvalue.  */
+  return (c->kind == ck_base && conv_is_prvalue (next_conversion (c)));
+}
+
+/* Return tristate::TS_TRUE if converting EXPR to a reference type TYPE binds
+   the reference to a temporary.  Return tristate::TS_FALSE if converting
+   EXPR to a reference type TYPE doesn't bind the reference to a temporary.  If
+   the conversion is invalid or bad, return tristate::TS_UNKNOWN.  
DIRECT_INIT_P
     says whether the conversion should be done in direct- or 
copy-initialization
     context.  */
tristate
-ref_conv_binds_directly (tree type, tree expr, bool direct_init_p /*= false*/)
+ref_conv_binds_to_temporary (tree type, tree expr, bool 
direct_init_p/*=false*/)
  {
    gcc_assert (TYPE_REF_P (type));
@@ -9230,7 +9251,7 @@ ref_conv_binds_directly (tree type, tree expr, bool direct_init_p /*= false*/)
                                          /*c_cast_p=*/false, flags, tf_none);
    tristate ret (tristate::TS_UNKNOWN);
    if (conv && !conv->bad_p)
-    ret = tristate (!conv_binds_ref_to_prvalue (conv));
+    ret = tristate (conv_binds_ref_to_temporary (conv));
/* Free all the conversions we allocated. */
    obstack_free (&conversion_obstack, p);
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 8bc1c2dc7fd..469eb2fdb25 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6534,7 +6534,7 @@ extern bool sufficient_parms_p                    
(const_tree);
  extern tree type_decays_to                    (tree);
  extern tree extract_call_expr                 (tree);
  extern tree build_trivial_dtor_call           (tree, bool = false);
-extern tristate ref_conv_binds_directly                (tree, tree, bool = 
false);
+extern tristate ref_conv_binds_to_temporary    (tree, tree, bool = false);
  extern tree build_user_type_conversion                (tree, tree, int,
                                                 tsubst_flags_t);
  extern tree build_new_function_call           (tree, vec<tree, va_gc> **,
diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc
index 55af5c43c18..622e1b9802e 100644
--- a/gcc/cp/method.cc
+++ b/gcc/cp/method.cc
@@ -2233,7 +2233,7 @@ ref_xes_from_temporary (tree to, tree from, bool 
direct_init_p)
    tree val = build_stub_object (from);
    if (!TYPE_REF_P (from) && TREE_CODE (from) != FUNCTION_TYPE)
      val = CLASS_TYPE_P (from) ? force_rvalue (val, tf_none) : rvalue (val);
-  return ref_conv_binds_directly (to, val, direct_init_p).is_false ();
+  return ref_conv_binds_to_temporary (to, val, direct_init_p).is_true ();
  }
/* Worker for is_{,nothrow_}convertible. Attempt to perform an implicit
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 90e06f914fb..bceedab5cd6 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -13731,7 +13731,8 @@ warn_for_range_copy (tree decl, tree expr)
if (TYPE_REF_P (type))
      {
-      if (glvalue_p (expr) && ref_conv_binds_directly (type, expr).is_false ())
+      if (glvalue_p (expr)
+         && ref_conv_binds_to_temporary (type, expr).is_true ())
        {
          auto_diagnostic_group d;
          if (warning_at (loc, OPT_Wrange_loop_construct,
@@ -13762,7 +13763,7 @@ warn_for_range_copy (tree decl, tree expr)
    /* If we can initialize a reference directly, suggest that to avoid the
       copy.  */
    tree rtype = cp_build_reference_type (type, /*rval*/false);
-  if (ref_conv_binds_directly (rtype, expr).is_true ())
+  if (ref_conv_binds_to_temporary (rtype, expr).is_false ())
      {
        auto_diagnostic_group d;
        if (warning_at (loc, OPT_Wrange_loop_construct,
diff --git a/gcc/testsuite/g++.dg/cpp0x/elision4.C 
b/gcc/testsuite/g++.dg/cpp0x/elision4.C
new file mode 100644
index 00000000000..3cc2e3afa5d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/elision4.C
@@ -0,0 +1,15 @@
+// PR c++/107085
+// { dg-do compile { target c++11 } }
+
+struct X {
+  X();
+  X(X&&);
+};
+struct Z : X {};
+X x1 = Z();
+X x2 = X(Z());
+
+struct B { };
+struct D : B { };
+B b1 = D();
+B b2 = B(D());
diff --git a/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C 
b/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
index 76de905a35d..5354b1dc4e6 100644
--- a/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
+++ b/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
@@ -201,7 +201,7 @@ SA(!__reference_constructs_from_temporary(const int&, H));
  SA(!__reference_constructs_from_temporary(int&&, G2));
  SA(!__reference_constructs_from_temporary(const int&, H2));
-SA(!__reference_constructs_from_temporary(const Base&, Der));
+SA(__reference_constructs_from_temporary(const Base&, Der));
// This fails because std::is_constructible_v<int&&, id<int[3]>> is false.
  SA(!__reference_constructs_from_temporary(int&&, id<int[3]>));
diff --git a/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C 
b/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
index 90196c38742..e6c159e9b00 100644
--- a/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
+++ b/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
@@ -201,7 +201,7 @@ SA( __reference_converts_from_temporary(const int&, H));
  SA(!__reference_converts_from_temporary(int&&, G2));
  SA(!__reference_converts_from_temporary(const int&, H2));
-SA(!__reference_converts_from_temporary(const Base&, Der));
+SA(__reference_converts_from_temporary(const Base&, Der));
// This fails because std::is_constructible_v<int&&, id<int[3]>> is false.
  SA(!__reference_converts_from_temporary(int&&, id<int[3]>));

base-commit: 50c35c691517291dbb77b1661761bc59950ba101

Reply via email to