Re: [PATCH] c++: more ahead-of-time -Wparentheses warnings

[Jason Merrill <jason@redhat.com>]

On 10/26/23 17:37, Patrick Palka wrote:
> On Thu, 26 Oct 2023, Patrick Palka wrote:
> 
>> On Thu, 26 Oct 2023, Jason Merrill wrote:
>>
>>> On 10/25/23 14:55, Patrick Palka wrote:
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
>>>> OK for trunk?
>>>>
>>>> -- >8 --
>>>>
>>>> Now that we don't have to worry about looking thruogh NON_DEPENDENT_EXPR,
>>>> we can easily extend the -Wparentheses warning in convert_for_assignment
>>>> to consider (non-dependent) templated assignment operator expressions as
>>>> well, like r14-4111-g6e92a6a2a72d3b did in maybe_convert_cond.
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	* cp-tree.h (is_assignment_op_expr_p): Declare.
>>>> 	* semantics.cc (is_assignment_op_expr_p): Generalize to return
>>>> 	true for assignment operator expression, not just one that
>>>> 	have been resolved to an operator overload.
>>>> 	(maybe_convert_cond): Remove now-redundant checks around
>>>> 	is_assignment_op_expr_p.
>>>> 	* typeck.cc (convert_for_assignment): Look through implicit
>>>> 	INDIRECT_REF in -Wparentheses warning logic, and generalize
>>>> 	to use is_assignment_op_expr_p.
>>>
>>> Do we want to factor out the whole warning logic rather than adjust it in both
>>> places?
>>
>> Sounds good, like so?  Bootstrap / regtest in progress.
>>
>> -- >8 --
>>
>> Subject: [PATCH] c++: more ahead-of-time -Wparentheses warnings
>>
>> Now that we don't have to worry about looking through NON_DEPENDENT_EXPR,
>> we can easily extend the -Wparentheses warning in convert_for_assignment
>> to consider (non-dependent) templated assignment operator expressions as
>> well, like r14-4111-g6e92a6a2a72d3b did in maybe_convert_cond.
>>
>> gcc/cp/ChangeLog:
>>
>> 	* cp-tree.h (maybe_warn_unparenthesized_assignment): Declare.
>> 	* semantics.cc (is_assignment_op_expr_p): Generalize to return
>> 	true for assignment operator expression, not just one that
>> 	have been resolved to an operator overload.
>> 	(maybe_warn_unparenthesized_assignment): Factored out from ...
>> 	(maybe_convert_cond): ... here.
>> 	(finish_parenthesized_expr): Also mention
>> 	maybe_warn_unparenthesized_assignment.
>> 	* typeck.cc (convert_for_assignment): Replace -Wparentheses
>> 	warning logic with maybe_warn_unparenthesized_assignment.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/warn/Wparentheses-13.C: Strengthen by expecting that
>> 	the -Wparentheses warning are issued ahead of time.
>> 	* g++.dg/warn/Wparentheses-23.C: Likewise.
>> 	* g++.dg/warn/Wparentheses-32.C: Remove xfails.
>> ---
>>   gcc/cp/cp-tree.h                            |  1 +
>>   gcc/cp/semantics.cc                         | 55 ++++++++++++++-------
>>   gcc/cp/typeck.cc                            | 13 ++---
>>   gcc/testsuite/g++.dg/warn/Wparentheses-13.C |  2 -
>>   gcc/testsuite/g++.dg/warn/Wparentheses-23.C |  3 --
>>   gcc/testsuite/g++.dg/warn/Wparentheses-32.C |  8 +--
>>   6 files changed, 44 insertions(+), 38 deletions(-)
>>
>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index 30fe716b109..98b29e9cf81 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -7875,6 +7875,7 @@ extern tree lambda_regenerating_args		(tree);
>>   extern tree most_general_lambda			(tree);
>>   extern tree finish_omp_target			(location_t, tree, tree, bool);
>>   extern void finish_omp_target_clauses		(location_t, tree, tree *);
>> +extern void maybe_warn_unparenthesized_assignment (tree, tsubst_flags_t);
>>   
>>   /* in tree.cc */
>>   extern int cp_tree_operand_length		(const_tree);
>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
>> index 72ec72de690..5664da9f4f2 100644
>> --- a/gcc/cp/semantics.cc
>> +++ b/gcc/cp/semantics.cc
>> @@ -840,15 +840,20 @@ finish_goto_stmt (tree destination)
>>     return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
>>   }
>>   
>> -/* Returns true if CALL is a (possibly wrapped) CALL_EXPR or AGGR_INIT_EXPR
>> -   to operator= () that is written as an operator expression. */
>> +/* Returns true if T corresponds to an assignment operator expression.  */
>> +
>>   static bool
>> -is_assignment_op_expr_p (tree call)
>> +is_assignment_op_expr_p (tree t)
>>   {
>> -  if (call == NULL_TREE)
>> +  if (t == NULL_TREE)
>>       return false;
>>   
>> -  call = extract_call_expr (call);
>> +  if (TREE_CODE (t) == MODIFY_EXPR
>> +      || (TREE_CODE (t) == MODOP_EXPR
>> +	  && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR))
>> +    return true;
>> +
>> +  tree call = extract_call_expr (t);
>>     if (call == NULL_TREE
>>         || call == error_mark_node
>>         || !CALL_EXPR_OPERATOR_SYNTAX (call))
>> @@ -860,6 +865,28 @@ is_assignment_op_expr_p (tree call)
>>       && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
>>   }
>>   
>> +/* Maybe warn about an unparenthesized 'a = b' (appearing in a boolean
>> +   context).  */
>> +
>> +void
>> +maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain)
>> +{
>> +  t = REFERENCE_REF_P (t) ? TREE_OPERAND (t, 0) : t;
> 
> Consider this to instead be written more idiomatically as

OK.

Jason