Re: [PATCH] c++: Remove maybe-rvalue OR in implicit move

[Marek Polacek <polacek@redhat.com>]

Ping.

On Wed, Sep 28, 2022 at 05:26:34PM -0400, Marek Polacek via Gcc-patches wrote:
> This patch removes the two-stage overload resolution when performing
> implicit move, whereby the compiler does two separate overload resolutions:
> one treating the operand as an rvalue, and then (if that resolution fails)
> another one treating the operand as an lvalue.  In the standard this was
> introduced via CWG 1579 and implemented in gcc in r251035.  In r11-2412,
> we disabled the fallback OR in C++20 (but not in C++17).  Then C++23 P2266
> removed the fallback overload resolution, and changed the implicit move rules
> once again.  So we wound up with three different behaviors.
> 
> The two overload resolutions approach was complicated and quirky, so
> users should transition to the newer model.  Removing the maybe-rvalue
> OR also allows us to simplify our code, for instance, now we can get
> rid of LOOKUP_PREFER_RVALUE altogether.
> 
> This change means that code that previously didn't compile in C++17 will
> now compile, for example:
> 
>   struct S1 { S1(S1 &&); };
>   struct S2 : S1 {};
> 
>   S1
>   f (S2 s)
>   {
>     return s; // OK, derived-to-base, use S1::S1(S1&&)
>   }
> 
> And conversely, code that used to work in C++17 may not compile anymore:
> 
>   struct W {
>     W();
>   };
> 
>   struct F {
>     F(W&);
>     F(W&&) = delete;
>   };
> 
>   F fn ()
>   {
>     W w;
>     return w; // use w as rvalue -> use of deleted function F::F(W&&)
>   }
> 
> I plan to add a note to porting_to.html.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (standard_conversion): Remove LOOKUP_PREFER_RVALUE code.
> 	(reference_binding): Honor clk_implicit_rval even pre-C++20.
> 	(implicit_conversion_1): Remove LOOKUP_PREFER_RVALUE code.
> 	(build_user_type_conversion_1): Likewise.
> 	(convert_like_internal): Likewise.
> 	(build_over_call): Likewise.
> 	* cp-tree.h (LOOKUP_PREFER_RVALUE): Remove.
> 	(LOOKUP_NO_NARROWING): Adjust definition.
> 	* except.cc (build_throw): Don't perform two overload resolutions.
> 	* typeck.cc (maybe_warn_pessimizing_move): Don't use
> 	LOOKUP_PREFER_RVALUE.
> 	(check_return_expr): Don't perform two overload resolutions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wredundant-move10.C: Adjust dg-warning.
> 	* g++.dg/cpp0x/Wredundant-move7.C: Likewise.
> 	* g++.dg/cpp0x/move-return2.C: Remove dg-error.
> 	* g++.dg/cpp0x/move-return4.C: Likewise.
> 	* g++.dg/cpp0x/ref-qual20.C: Adjust expected return value.
> 	* g++.dg/cpp0x/move-return5.C: New test.
> ---
>  gcc/cp/call.cc                                | 41 ++-----------------
>  gcc/cp/cp-tree.h                              |  6 +--
>  gcc/cp/except.cc                              | 23 ++---------
>  gcc/cp/typeck.cc                              | 34 +++------------
>  .../g++.dg/cpp0x/Wredundant-move10.C          |  2 +-
>  gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C |  6 +--
>  gcc/testsuite/g++.dg/cpp0x/move-return2.C     |  2 +-
>  gcc/testsuite/g++.dg/cpp0x/move-return4.C     |  2 +-
>  gcc/testsuite/g++.dg/cpp0x/move-return5.C     | 20 +++++++++
>  gcc/testsuite/g++.dg/cpp0x/ref-qual20.C       |  2 +-
>  10 files changed, 41 insertions(+), 97 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/move-return5.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 3506b0fcfbb..e100913ea29 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -1272,9 +1272,6 @@ standard_conversion (tree to, tree from, tree expr, bool c_cast_p,
>  	    }
>  	}
>        conv = build_conv (ck_rvalue, from, conv);
> -      if (flags & LOOKUP_PREFER_RVALUE)
> -	/* Tell convert_like to set LOOKUP_PREFER_RVALUE.  */
> -	conv->rvaluedness_matches_p = true;
>        /* If we're performing copy-initialization, remember to skip
>  	 explicit constructors.  */
>        if (flags & LOOKUP_ONLYCONVERTING)
> @@ -1572,9 +1569,6 @@ standard_conversion (tree to, tree from, tree expr, bool c_cast_p,
>  	 type.  A temporary object is created to hold the result of
>  	 the conversion unless we're binding directly to a reference.  */
>        conv->need_temporary_p = !(flags & LOOKUP_NO_TEMP_BIND);
> -      if (flags & LOOKUP_PREFER_RVALUE)
> -	/* Tell convert_like to set LOOKUP_PREFER_RVALUE.  */
> -	conv->rvaluedness_matches_p = true;
>        /* If we're performing copy-initialization, remember to skip
>  	 explicit constructors.  */
>        if (flags & LOOKUP_ONLYCONVERTING)
> @@ -1883,7 +1877,7 @@ reference_binding (tree rto, tree rfrom, tree expr, bool c_cast_p, int flags,
>  	  /* Unless it's really a C++20 lvalue being treated as an xvalue.
>  	     But in C++23, such an expression is just an xvalue, not a special
>  	     lvalue, so the binding is once again ill-formed.  */
> -	  && !(cxx_dialect == cxx20
> +	  && !(cxx_dialect <= cxx20
>  	       && (gl_kind & clk_implicit_rval))
>  	  && (!CP_TYPE_CONST_NON_VOLATILE_P (to)
>  	      || (flags & LOOKUP_NO_RVAL_BIND))
> @@ -2044,9 +2038,8 @@ implicit_conversion_1 (tree to, tree from, tree expr, bool c_cast_p,
>    /* Other flags only apply to the primary function in overload
>       resolution, or after we've chosen one.  */
>    flags &= (LOOKUP_ONLYCONVERTING|LOOKUP_NO_CONVERSION|LOOKUP_COPY_PARM
> -	    |LOOKUP_NO_TEMP_BIND|LOOKUP_NO_RVAL_BIND|LOOKUP_PREFER_RVALUE
> -	    |LOOKUP_NO_NARROWING|LOOKUP_PROTECT|LOOKUP_NO_NON_INTEGRAL
> -	    |LOOKUP_SHORTCUT_BAD_CONVS);
> +	    |LOOKUP_NO_TEMP_BIND|LOOKUP_NO_RVAL_BIND|LOOKUP_NO_NARROWING
> +	    |LOOKUP_PROTECT|LOOKUP_NO_NON_INTEGRAL|LOOKUP_SHORTCUT_BAD_CONVS);
>  
>    /* FIXME: actually we don't want warnings either, but we can't just
>       have 'complain &= ~(tf_warning|tf_error)' because it would cause
> @@ -4451,14 +4444,6 @@ 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.  Reject converting the return
> -     value of the conversion function to a base class.  */
> -  if ((flags & LOOKUP_PREFER_RVALUE) && !DECL_CONSTRUCTOR_P (cand->fn))
> -    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)
>      conv->check_narrowing = true;
> @@ -8281,9 +8266,6 @@ convert_like_internal (conversion *convs, tree expr, tree fn, int argnum,
>  	 explicit constructors.  */
>        if (convs->copy_init_p)
>  	flags |= LOOKUP_ONLYCONVERTING;
> -      if (convs->rvaluedness_matches_p)
> -	/* standard_conversion got LOOKUP_PREFER_RVALUE.  */
> -	flags |= LOOKUP_PREFER_RVALUE;
>        expr = build_temp (expr, totype, flags, &diag_kind, complain);
>        if (diag_kind && complain)
>  	{
> @@ -9529,23 +9511,6 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
>  	  ++arg_index;
>  	  parm = TREE_CHAIN (parm);
>  	}
> -
> -      if (cxx_dialect < cxx20
> -	  && (cand->flags & LOOKUP_PREFER_RVALUE))
> -	{
> -	  /* The implicit move specified in 15.8.3/3 fails "...if the type of
> -	     the first parameter of the selected constructor is not an rvalue
> -	     reference to the object's type (possibly cv-qualified)...." */
> -	  gcc_assert (!(complain & tf_error));
> -	  tree ptype = convs[0]->type;
> -	  /* Allow calling a by-value converting constructor even though it
> -	     isn't permitted by the above, because we've allowed it since GCC 5
> -	     (PR58051) and it's allowed in C++20.  But don't call a copy
> -	     constructor.  */
> -	  if ((TYPE_REF_P (ptype) && !TYPE_REF_IS_RVALUE (ptype))
> -	      || CONVERSION_RANK (convs[0]) > cr_exact)
> -	    return error_mark_node;
> -	}
>      }
>    /* Bypass access control for 'this' parameter.  */
>    else if (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE)
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index d0f1b18b015..7acb5373513 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5853,12 +5853,8 @@ enum overload_flags { NO_SPECIAL = 0, DTOR_FLAG, TYPENAME_FLAG };
>  #define LOOKUP_DESTRUCTOR (1 << 5)
>  /* Do not permit references to bind to temporaries.  */
>  #define LOOKUP_NO_TEMP_BIND (1 << 6)
> -/* We're trying to treat an lvalue as an rvalue.  */
> -/* FIXME remove when we extend the P1825 semantics to all standard modes, the
> -   C++20 approach uses IMPLICIT_RVALUE_P instead.  */
> -#define LOOKUP_PREFER_RVALUE (LOOKUP_NO_TEMP_BIND << 1)
>  /* We're inside an init-list, so narrowing conversions are ill-formed.  */
> -#define LOOKUP_NO_NARROWING (LOOKUP_PREFER_RVALUE << 1)
> +#define LOOKUP_NO_NARROWING (LOOKUP_NO_TEMP_BIND << 1)
>  /* We're looking up a constructor for list-initialization.  */
>  #define LOOKUP_LIST_INIT_CTOR (LOOKUP_NO_NARROWING << 1)
>  /* This is the first parameter of a copy constructor.  */
> diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> index 048612de400..1d84b451c9f 100644
> --- a/gcc/cp/except.cc
> +++ b/gcc/cp/except.cc
> @@ -715,25 +715,10 @@ build_throw (location_t loc, tree exp)
>  	     treated as an rvalue for the purposes of overload resolution
>  	     to favor move constructors over copy constructors.  */
>  	  if (tree moved = treat_lvalue_as_rvalue_p (exp, /*return*/false))
> -	    {
> -	      if (cxx_dialect < cxx20)
> -		{
> -		  releasing_vec exp_vec (make_tree_vector_single (moved));
> -		  moved = (build_special_member_call
> -			   (object, complete_ctor_identifier, &exp_vec,
> -			    TREE_TYPE (object), flags|LOOKUP_PREFER_RVALUE,
> -			    tf_none));
> -		  if (moved != error_mark_node)
> -		    {
> -		      exp = moved;
> -		      converted = true;
> -		    }
> -		}
> -	      else
> -		/* In C++20 we just treat the return value as an rvalue that
> -		   can bind to lvalue refs.  */
> -		exp = moved;
> -	    }
> +	    /* In C++20 we just treat the return value as an rvalue that
> +	       can bind to lvalue refs.  In C++23, such an expression is just
> +	       an xvalue.  */
> +	    exp = moved;
>  
>  	  /* Call the copy constructor.  */
>  	  if (!converted)
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 5f16c4d2426..c525e880383 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10692,21 +10692,12 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>  	  tree t = convert_for_initialization (NULL_TREE, type,
>  					       moved,
>  					       (LOOKUP_NORMAL
> -						| LOOKUP_ONLYCONVERTING
> -						| LOOKUP_PREFER_RVALUE),
> +						| LOOKUP_ONLYCONVERTING),
>  					       ICR_RETURN, NULL_TREE, 0,
>  					       tf_none);
>  	  /* If this worked, implicit rvalue would work, so the call to
>  	     std::move is redundant.  */
> -	  if (t != error_mark_node
> -	      /* Trying to move something const will never succeed unless
> -		 there's T(const T&&), which it almost never is, and if
> -		 so, T wouldn't be error_mark_node now: the above convert_
> -		 call with LOOKUP_PREFER_RVALUE returns an error if a const T&
> -		 overload is selected.  */
> -	      || (CP_TYPE_CONST_P (TREE_TYPE (arg))
> -		  && same_type_ignoring_top_level_qualifiers_p
> -		  (TREE_TYPE (arg), type)))
> +	  if (t != error_mark_node)
>  	    {
>  	      auto_diagnostic_group d;
>  	      if (warning_at (loc, OPT_Wredundant_move,
> @@ -11049,23 +11040,10 @@ check_return_expr (tree retval, bool *no_warning)
>  	     ? CLASS_TYPE_P (functype)
>  	     : !SCALAR_TYPE_P (functype) || !SCALAR_TYPE_P (TREE_TYPE (retval)))
>  	    && (moved = treat_lvalue_as_rvalue_p (retval, /*return*/true)))
> -	{
> -	  if (cxx_dialect < cxx20)
> -	    {
> -	      moved = convert_for_initialization
> -		(NULL_TREE, functype, moved, flags|LOOKUP_PREFER_RVALUE,
> -		 ICR_RETURN, NULL_TREE, 0, tf_none);
> -	      if (moved != error_mark_node)
> -		{
> -		  retval = moved;
> -		  converted = true;
> -		}
> -	    }
> -	  else
> -	    /* In C++20 we just treat the return value as an rvalue that
> -	       can bind to lvalue refs.  */
> -	    retval = moved;
> -	}
> +	  /* In C++20 and earlier we just treat the return value as an rvalue
> +	     that can bind to lvalue refs.  In C++23, such an expression is just
> +	     an xvalue (see reference_binding).  */
> +	  retval = moved;
>  
>        /* The call in a (lambda) thunk needs no conversions.  */
>        if (TREE_CODE (retval) == CALL_EXPR
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> index a215a4774d6..17dd807aea8 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> @@ -57,5 +57,5 @@ struct S2: S1 {};
>  
>  S1 f3(const S2 s)
>  {
> -  return std::move(s); // { dg-warning "redundant move" "" { target c++20 } }
> +  return std::move(s); // { dg-warning "redundant move" }
>  }
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C
> index 3fec525879d..6547777c007 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C
> @@ -28,7 +28,7 @@ struct S2 : S1 {};
>  S1
>  f (S2 s)
>  {
> -  return std::move(s); // { dg-warning "redundant move in return statement" "" { target c++20 } }
> +  return std::move(s); // { dg-warning "redundant move in return statement" }
>  }
>  
>  struct R1 {
> @@ -40,7 +40,7 @@ struct R2 : R1 {};
>  R1
>  f2 (const R2 s)
>  {
> -  return std::move(s); // { dg-warning "redundant move in return statement" "" { target c++20 } }
> +  return std::move(s); // { dg-warning "redundant move in return statement" }
>  }
>  
>  struct T1 {
> @@ -55,5 +55,5 @@ f3 (const T2 s)
>  {
>    // Without std::move: const T1 &
>    // With std::move: const T1 &&
> -  return std::move(s); // { dg-warning "redundant move in return statement" "" { target c++20 } }
> +  return std::move(s); // { dg-warning "redundant move in return statement" }
>  }
> diff --git a/gcc/testsuite/g++.dg/cpp0x/move-return2.C b/gcc/testsuite/g++.dg/cpp0x/move-return2.C
> index 999f2c95c49..8e750efb870 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/move-return2.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/move-return2.C
> @@ -7,5 +7,5 @@ struct S2 : S1 {};
>  S1
>  f (S2 s)
>  {
> -  return s; // { dg-error "use of deleted function" "" { target c++17_down } }
> +  return s;
>  }
> diff --git a/gcc/testsuite/g++.dg/cpp0x/move-return4.C b/gcc/testsuite/g++.dg/cpp0x/move-return4.C
> index 3fc58089319..0f0ca1fc253 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/move-return4.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/move-return4.C
> @@ -13,5 +13,5 @@ struct A : Base
>  A<int> foo()
>  {
>      A<double> v;
> -    return v; // { dg-error "cannot bind rvalue reference" "" { target c++17_down } }
> +    return v;
>  }
> diff --git a/gcc/testsuite/g++.dg/cpp0x/move-return5.C b/gcc/testsuite/g++.dg/cpp0x/move-return5.C
> new file mode 100644
> index 00000000000..695000b2687
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/move-return5.C
> @@ -0,0 +1,20 @@
> +// { dg-do compile { target c++11 } }
> +// This used to compile in C++11...17 because we performed two
> +// separate overload resolutions: one treating the operand as
> +// an rvalue, and then (if that resolution fails) another one
> +// treating the operand as an lvalue.
> +
> +struct W {
> +  W();
> +};
> +
> +struct F {
> +  F(W&);
> +  F(W&&) = delete;
> +};
> +
> +F fn ()
> +{
> +  W w;
> +  return w; // { dg-error "use of deleted function" }
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/ref-qual20.C b/gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
> index cfbef300226..314f19bb864 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/ref-qual20.C
> @@ -52,7 +52,7 @@ f5 ()
>  int
>  main ()
>  {
> -  int return_lval = __cplusplus > 201703L ? -1 : 2;
> +  int return_lval = -1;
>    Y y1 = f (A());
>    if (y1.y != return_lval)
>      __builtin_abort ();
> 
> base-commit: 9f65eecdbef6027722e93aadf3fa6b3cc342eb4f
> -- 
> 2.37.3
> 

Marek