[RFC] c++: don't call 'rvalue' in coroutines code

Message ID 20210507031156.4023693-1-jason@redhat.com
State New
Headers show
Series
  • [RFC] c++: don't call 'rvalue' in coroutines code
Related show

Commit Message

Andre Vehreschild via Gcc-patches May 7, 2021, 3:11 a.m.
A change to check glvalue_p rather than specifically for TARGET_EXPR
revealed issues with the coroutines code's use of the 'rvalue' function,
which shouldn't be used on class glvalues, so I've removed those calls.

In build_co_await I just dropped them, because I don't see anything in the
co_await specification that indicates that we would want to move from an
lvalue result of operator co_await.  And simplified that code while I was
touching it; cp_build_modify_expr (...INIT_EXPR...) will call the
constructor.

In morph_fn_to_coro I changed the handling of the rvalue reference coroutine
frame field to use move, to treat the rval ref as an xvalue.  I used
forward_parm to pass the function parms to the constructor for the field.
And I simplified the return handling so we get the desired rvalue semantics
from the normal implicit move on return.

Iain, does this all make sense to you?

Incidentally, what is the standard citation for default-initializing the
non-void return value of the function if get_return_object returns void?
All I see is "The expression /promise/.get_return_object() is used to
initialize the glvalue result or prvalue result object of a call to a
coroutine", which suggests to me that that situation should be ill-formed.

gcc/cp/ChangeLog:

	* coroutines.cc (build_co_await): Don't call 'rvalue'.
	(flatten_await_stmt): Simplify initialization.
	(morph_fn_to_coro): Change 'rvalue' to 'move'.  Simplify.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/coro-bad-gro-00-class-gro-scalar-return.C:
	Adjust diagnostic.
---
 gcc/cp/coroutines.cc                          | 117 +++++-------------
 .../coro-bad-gro-00-class-gro-scalar-return.C |   2 +-
 2 files changed, 31 insertions(+), 88 deletions(-)


base-commit: e82e87a851cdea9f4f43f342842025b068287d4e
-- 
2.27.0

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index dbd703a67cc..03543d5c112 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -950,18 +950,11 @@  build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind)
       e_proxy = o;
       o = NULL_TREE; /* The var is already present.  */
     }
-  else if (type_build_ctor_call (o_type))
-    {
-      e_proxy = get_awaitable_var (suspend_kind, o_type);
-      releasing_vec arg (make_tree_vector_single (rvalue (o)));
-      o = build_special_member_call (e_proxy, complete_ctor_identifier,
-				     &arg, o_type, LOOKUP_NORMAL,
-				     tf_warning_or_error);
-    }
   else
     {
       e_proxy = get_awaitable_var (suspend_kind, o_type);
-      o = build2 (INIT_EXPR, o_type, e_proxy, rvalue (o));
+      o = cp_build_modify_expr (loc, e_proxy, INIT_EXPR, o,
+				tf_warning_or_error);
     }
 
   /* I suppose we could check that this is contextually convertible to bool.  */
@@ -2989,15 +2982,8 @@  flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
 	  gcc_checking_assert (!already_present);
 	  tree inner = TREE_OPERAND (init, 1);
 	  gcc_checking_assert (TREE_CODE (inner) != COND_EXPR);
-	  if (type_build_ctor_call (var_type))
-	    {
-	      releasing_vec p_in (make_tree_vector_single (init));
-	      init = build_special_member_call (var, complete_ctor_identifier,
-						&p_in, var_type, LOOKUP_NORMAL,
-						tf_warning_or_error);
-	    }
-	  else
-	    init = build2 (INIT_EXPR, var_type, var, init);
+	  init = cp_build_modify_expr (input_location, var, INIT_EXPR, init,
+				       tf_warning_or_error);
 	  /* Simplify for the case that we have an init containing the temp
 	     alone.  */
 	  if (t == n->init && n->var == NULL_TREE)
@@ -4862,43 +4848,19 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 	      vec_safe_push (promise_args, this_ref);
 	    }
 	  else if (parm.rv_ref)
-	    vec_safe_push (promise_args, rvalue(fld_idx));
+	    vec_safe_push (promise_args, move (fld_idx));
 	  else
 	    vec_safe_push (promise_args, fld_idx);
 
 	  if (parm.rv_ref || parm.pt_ref)
 	    /* Initialise the frame reference field directly.  */
-	    r = build_modify_expr (fn_start, TREE_OPERAND (fld_idx, 0),
-				   parm.frame_type, INIT_EXPR,
-				   DECL_SOURCE_LOCATION (arg), arg,
-				   DECL_ARG_TYPE (arg));
-	  else if (type_build_ctor_call (parm.frame_type))
-	    {
-	      vec<tree, va_gc> *p_in;
-	      if (CLASS_TYPE_P (parm.frame_type)
-		  && classtype_has_non_deleted_move_ctor (parm.frame_type))
-		p_in = make_tree_vector_single (move (arg));
-	      else if (lvalue_p (arg))
-		p_in = make_tree_vector_single (rvalue (arg));
-	      else
-		p_in = make_tree_vector_single (arg);
-	      /* Construct in place or move as relevant.  */
-	      r = build_special_member_call (fld_idx, complete_ctor_identifier,
-					     &p_in, parm.frame_type,
-					     LOOKUP_NORMAL,
-					     tf_warning_or_error);
-	      release_tree_vector (p_in);
-	    }
+	    r = cp_build_modify_expr (fn_start, TREE_OPERAND (fld_idx, 0),
+				      INIT_EXPR, arg, tf_warning_or_error);
 	  else
 	    {
-	      if (!same_type_p (parm.frame_type, DECL_ARG_TYPE (arg)))
-		r = build1_loc (DECL_SOURCE_LOCATION (arg), CONVERT_EXPR,
-				parm.frame_type, arg);
-	      else
-		r = arg;
-	      r = build_modify_expr (fn_start, fld_idx, parm.frame_type,
-				     INIT_EXPR, DECL_SOURCE_LOCATION (arg), r,
-				     TREE_TYPE (r));
+	      r = forward_parm (arg);
+	      r = cp_build_modify_expr (fn_start, fld_idx, INIT_EXPR, r,
+					tf_warning_or_error);
 	    }
 	  finish_expr_stmt (r);
 	  if (!parm.trivial_dtor)
@@ -5044,16 +5006,8 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
       DECL_IGNORED_P (gro) = true;
       add_decl_expr (gro);
       gro_bind_vars = gro;
-      if (type_build_ctor_call (gro_type))
-	{
-	  vec<tree, va_gc> *arg = make_tree_vector_single (get_ro);
-	  r = build_special_member_call (gro, complete_ctor_identifier,
-					 &arg, gro_type, LOOKUP_NORMAL,
-					 tf_warning_or_error);
-	  release_tree_vector (arg);
-	}
-      else
-	r = build2_loc (fn_start, INIT_EXPR, gro_type, gro, get_ro);
+      r = cp_build_modify_expr (input_location, gro, INIT_EXPR, get_ro,
+				tf_warning_or_error);
       /* The constructed object might require a cleanup.  */
       if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (gro_type))
 	{
@@ -5111,37 +5065,26 @@  morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
 
   if (same_type_p (gro_type, fn_return_type))
     r = gro_is_void_p ? NULL_TREE : DECL_RESULT (orig);
+  else if (!gro_is_void_p)
+    /* check_return_expr will automatically return gro as an rvalue via
+       treat_lvalue_as_rvalue_p.  */
+    r = gro;
+  else if (CLASS_TYPE_P (fn_return_type))
+    {
+      /* For class type return objects, we can attempt to construct,
+	 even if the gro is void.  */
+      r = build_special_member_call (NULL_TREE,
+				     complete_ctor_identifier, NULL,
+				     fn_return_type, LOOKUP_NORMAL,
+				     tf_warning_or_error);
+      r = build_cplus_new (fn_return_type, r, tf_warning_or_error);
+    }
   else
     {
-      if (CLASS_TYPE_P (fn_return_type))
-	{
-	  /* For class type return objects, we can attempt to construct,
-	     even if the gro is void.  */
-	  vec<tree, va_gc> *args = NULL;
-	  vec<tree, va_gc> **arglist = NULL;
-	  if (!gro_is_void_p)
-	    {
-	      args = make_tree_vector_single (rvalue (gro));
-	      arglist = &args;
-	    }
-	  r = build_special_member_call (NULL_TREE,
-					 complete_ctor_identifier, arglist,
-					 fn_return_type, LOOKUP_NORMAL,
-					 tf_warning_or_error);
-	  r = build_cplus_new (fn_return_type, r, tf_warning_or_error);
-	  if (args)
-	    release_tree_vector (args);
-	}
-      else if (gro_is_void_p)
-	{
-	  /* We can't initialize a non-class return value from void.  */
-	  error_at (input_location, "cannot initialize a return object of type"
-		    " %qT with an rvalue of type %<void%>", fn_return_type);
-	  r = error_mark_node;
-	}
-      else
-	r = build1_loc (input_location, CONVERT_EXPR,
-			fn_return_type, rvalue (gro));
+      /* We can't initialize a non-class return value from void.  */
+      error_at (input_location, "cannot initialize a return object of type"
+		" %qT with an rvalue of type %<void%>", fn_return_type);
+      r = error_mark_node;
     }
 
   finish_return_stmt (r);
diff --git a/gcc/testsuite/g++.dg/coroutines/coro-bad-gro-00-class-gro-scalar-return.C b/gcc/testsuite/g++.dg/coroutines/coro-bad-gro-00-class-gro-scalar-return.C
index f9e8a5f398b..0512f03f4d0 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro-bad-gro-00-class-gro-scalar-return.C
+++ b/gcc/testsuite/g++.dg/coroutines/coro-bad-gro-00-class-gro-scalar-return.C
@@ -37,7 +37,7 @@  my_coro (std::coroutine_handle<>& h)
 {
   PRINT ("coro1: about to return");
   co_return;
-} // { dg-error {'struct Thing' used where a 'int' was expected} }
+} // { dg-error {cannot convert 'Thing' to 'int' in return} }
 
 int main ()
 {