Re: [PATCH] c++: non-dependent assignment checking [PR63198, PR18474]

[Jason Merrill <jason@redhat.com>]

On 9/17/23 14:51, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?  Patch generatde with -w to avoid noisy whitespace changes.
> 
> -- >8 --
> 
> This patch makes us recognize and check non-dependent simple assigments
> ahead of time, like we already do for compound assignments.  This means
> the templated representation of such assignments will now usually have
> an implicit INDIRECT_REF (due to the reference return type), which the
> -Wparentheses code needs to handle.  As a drive-by improvement, this
> patch also makes maybe_convert_cond issue -Wparentheses warnings ahead
> of time.
> 
> This revealed some libstdc++ tests were attempting to modify a data
> member from a uninstantiated const member function; naively fixed by
> making the data member mutable.
> 
> 	PR c++/63198
> 	PR c++/18474
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (maybe_convert_cond): Look through implicit
> 	INDIRECT_REF when deciding whether to issue a -Wparentheses
> 	warning, and consider templated assignment expressions as well.
> 	(finish_parenthesized_expr): Look through implicit INDIRECT_REF
> 	when suppressing -Wparentheses warning.
> 	* typeck.cc (build_x_modify_expr): Check simple assignments
> 	ahead time too, not just compound assignments.  Give the second
> 	operand of MODOP_EXPR a non-null type so that it's not considered
> 	always instantiation-dependent.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/static_assert15.C: Expect diagnostic for
> 	non-constant static_assert condition.
> 	* g++.dg/expr/unary2.C: Remove xfails.
> 	* g++.dg/template/init7.C: Make initializer type-dependent to
> 	preserve intent of test.
> 	* g++.dg/template/recurse3.C: Likewise for the erroneous
> 	statement.
> 	* g++.dg/template/non-dependent26.C: New test.
> 	* g++.dg/warn/Wparentheses-32.C: New test.
> 
> libstdc++/ChangeLog:
> 
> 	* testsuite/26_numerics/random/discard_block_engine/cons/seed_seq2.cc:
> 	Make seed_seq::called member mutable.
> 	* testsuite/26_numerics/random/independent_bits_engine/cons/seed_seq2.cc:
> 	Likewise.
> 	* testsuite/26_numerics/random/linear_congruential_engine/cons/seed_seq2.cc
> 	Likewise.
> 	* testsuite/26_numerics/random/mersenne_twister_engine/cons/seed_seq2.cc:
> 	Likewise.
> 	* testsuite/26_numerics/random/shuffle_order_engine/cons/seed_seq2.cc:
> 	Likewise.
> 	* testsuite/26_numerics/random/subtract_with_carry_engine/cons/seed_seq2.cc:
> 	Likewise.
> 	* testsuite/ext/random/simd_fast_mersenne_twister_engine/cons/seed_seq2.cc
> 	Likewise.
> ---
>   gcc/cp/semantics.cc                           | 17 +++++++----
>   gcc/cp/typeck.cc                              | 23 +++++++--------
>   gcc/testsuite/g++.dg/cpp0x/static_assert15.C  |  2 +-
>   gcc/testsuite/g++.dg/expr/unary2.C            |  8 ++----
>   gcc/testsuite/g++.dg/template/init7.C         |  2 +-
>   .../g++.dg/template/non-dependent26.C         | 25 +++++++++++++++++
>   gcc/testsuite/g++.dg/template/recurse3.C      |  8 +++---
>   gcc/testsuite/g++.dg/warn/Wparentheses-32.C   | 28 +++++++++++++++++++
>   .../discard_block_engine/cons/seed_seq2.cc    |  2 +-
>   .../independent_bits_engine/cons/seed_seq2.cc |  2 +-
>   .../cons/seed_seq2.cc                         |  2 +-
>   .../mersenne_twister_engine/cons/seed_seq2.cc |  2 +-
>   .../shuffle_order_engine/cons/seed_seq2.cc    |  2 +-
>   .../cons/seed_seq2.cc                         |  2 +-
>   .../cons/seed_seq2.cc                         |  2 +-
>   15 files changed, 91 insertions(+), 36 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/template/non-dependent26.C
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-32.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0f7f4e87ae4..b57c1ac868b 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 459739d5866..74f5fced060 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -9739,15 +9739,15 @@ build_x_modify_expr (location_t loc, tree lhs, enum tree_code modifycode,
>         rhs = build_non_dependent_expr (rhs);
>       }
>   
> -  if (modifycode != NOP_EXPR)
> -    {
> -      tree op = build_nt (modifycode, NULL_TREE, NULL_TREE);
> -      tree rval = build_new_op (loc, MODIFY_EXPR, LOOKUP_NORMAL,
> +  tree rval;
> +  if (modifycode == NOP_EXPR)
> +    rval = cp_build_modify_expr (loc, lhs, modifycode, rhs, complain);
> +  else
> +    rval = build_new_op (loc, MODIFY_EXPR, LOOKUP_NORMAL,
>   			 lhs, rhs, op, lookups, &overload, complain);
> -      if (rval)
> -	{
>     if (rval == error_mark_node)
> -	    return rval;
> +    return error_mark_node;
> +  if (modifycode != NOP_EXPR)
>       suppress_warning (rval /* What warning? */);

Did you try disabling this to see if it's still needed?

Jason