| Message ID | 20231220220749.632100-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: fix -Wparentheses with boolean-like class types | expand |
On 12/20/23 17:07, Patrick Palka wrote: > Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this > look OK for trunk if successful? > > -- >8 -- > > Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably > warns on the idiom > > Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses] > 9 | b = v[i] = true; > | ^~~~ > > where v has type std::vector<bool>. That commit intended to only extend > the existing diagnostics so that they happen in a template context as > well, but the refactoring of is_assignment_op_expr_p caused us for this > particular -Wparentheses warning (from convert_for_assignment) to now > consider user-defined operator= instead of just built-in operator=. And > since std::vector<bool> is really a bitset, whose operator[] returns a > class type with such a user-defined operator= (taking bool), we now warn. > > But arguably "boolish" class types should be treated like ordinary bool > as far as the warning is concerned. To that end this patch relaxes the > warning for such types, specifically when the (class) type can be > (implicitly) converted to a and assigned from a bool. This should cover > at least implementations of std::vector<bool>::reference. > > gcc/cp/ChangeLog: > > * cp-tree.h (maybe_warn_unparenthesized_assignment): Add > 'nested_p' bool parameter. > * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool > parameter and set it accordingly. > (class_type_is_boolish_cache): Define. > (class_type_is_boolish): Define. > (maybe_warn_unparenthesized_assignment): Add 'nested_p' > bool parameter. Relax the warning for nested assignments > to boolean-like class types. > (maybe_convert_cond): Pass nested_p=false to > maybe_warn_unparenthesized_assignment. > * typeck.cc (convert_for_assignment): Pass nested_p=true to > maybe_warn_unparenthesized_assignment. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-34.C: New test. > --- > gcc/cp/cp-tree.h | 2 +- > gcc/cp/semantics.cc | 106 ++++++++++++++++++-- > gcc/cp/typeck.cc | 2 +- > gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ > 4 files changed, 129 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 1979572c365..97065cccf3d 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7928,7 +7928,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); > +extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t); > extern tree cp_check_pragma_unroll (location_t, tree); > > /* in tree.cc */ > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 64839b1ac87..92acd560fa4 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) > return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); > } > > -/* Returns true if T corresponds to an assignment operator expression. */ > +/* Returns true if T corresponds to an assignment operator expression, > + and sets *LHS to its left-hand-side operand if so. */ > > static bool > -is_assignment_op_expr_p (tree t) > +is_assignment_op_expr_p (tree t, tree *lhs) > { > if (t == NULL_TREE) > return false; > @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) > if (TREE_CODE (t) == MODIFY_EXPR > || (TREE_CODE (t) == MODOP_EXPR > && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) > - return true; > + { > + *lhs = TREE_OPERAND (t, 0); > + return true; > + } > > tree call = extract_call_expr (t); > if (call == NULL_TREE > @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) > return false; > > tree fndecl = cp_get_callee_fndecl_nofold (call); > - return fndecl != NULL_TREE > - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); > + if (fndecl != NULL_TREE > + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) > + { > + *lhs = get_nth_callarg (call, 0); > + return true; > + } > + > + return false; > } > > +static GTY((deletable)) hash_map<tree, bool> *class_type_is_boolish_cache; > + > +/* Return true if the class type TYPE can be converted to and assigned > + from a boolean. */ > + > +static bool > +class_type_is_boolish (tree type) > +{ > + type = TYPE_MAIN_VARIANT (type); > + if (!COMPLETE_TYPE_P (type)) > + return false; > + > + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) > + return *r; > + > + bool has_bool_conversion = false; > + bool has_bool_assignment = false; > + > + tree ops = lookup_conversions (type); > + for (; ops; ops = TREE_CHAIN (ops)) > + { > + tree op = TREE_VALUE (ops); > + if (!DECL_NONCONVERTING_P (op) > + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) > + { > + has_bool_conversion = true; > + break; > + } > + } > + > + if (!has_bool_conversion) > + { > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, false); > + return false; > + } > + > + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, tf_none); > + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) > + { > + op = STRIP_TEMPLATE (op); > + if (TREE_CODE (op) != FUNCTION_DECL) > + continue; > + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); > + tree parm_type = non_reference (TREE_TYPE (parm)); > + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) > + { > + has_bool_assignment = true; > + break; > + } > + } > + > + bool boolish = has_bool_conversion && has_bool_assignment; > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); > + return boolish; > +} > + > + > /* Maybe warn about an unparenthesized 'a = b' (appearing in a > - boolean context where 'a == b' might have been intended). */ > + boolean context where 'a == b' might have been intended). > + NESTED_P is true if T appears as the RHS of another assignment. */ > > void > -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) > +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, > + tsubst_flags_t complain) > { > t = STRIP_REFERENCE_REF (t); > > + tree lhs; > if ((complain & tf_warning) > && warn_parentheses > - && is_assignment_op_expr_p (t) > + && is_assignment_op_expr_p (t, &lhs) > /* A parenthesized expression would've had this warning > suppressed by finish_parenthesized_expr. */ > && !warning_suppressed_p (t, OPT_Wparentheses)) > { > + /* In a = b = c, don't warn if b is a boolean-like class type. */ > + if (nested_p) > + { > + /* The caller already checked this. */ > + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and bool-ish type here. Can we check both in one place or the other? > + > + if (TREE_CODE (lhs) == ADDR_EXPR) > + lhs = TREE_OPERAND (lhs, 0); > + > + tree lhs_type = non_reference (TREE_TYPE (lhs)); > + if (CLASS_TYPE_P (lhs_type) > + && class_type_is_boolish (lhs_type)) > + return; > + } > + > warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses, > "suggest parentheses around assignment used as truth value"); > suppress_warning (t, OPT_Wparentheses); > @@ -903,7 +988,8 @@ maybe_convert_cond (tree cond) > if (warn_sequence_point && !processing_template_decl) > verify_sequence_points (cond); > > - maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error); > + maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, > + tf_warning_or_error); > > /* Do the conversion. */ > cond = convert_from_reference (cond); > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index a6e2f4ee7da..195114e716e 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10381,7 +10381,7 @@ convert_for_assignment (tree type, tree rhs, > does not. */ > if (TREE_CODE (type) == BOOLEAN_TYPE > && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE) > - maybe_warn_unparenthesized_assignment (rhs, complain); > + maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain); > > if (complain & tf_warning) > warn_for_address_of_packed_member (type, rhs); > diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C > new file mode 100644 > index 00000000000..2100c8a193d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C > @@ -0,0 +1,31 @@ > +// Verify our -Wparentheses warning handles "boolish" class types > +// such as std::vector<bool>'s reference type the same as ordinary > +// bool. > +// { dg-additional-options "-Wparentheses" } > + > +#include <vector> > + > +void f(std::vector<bool> v, int i) { > + bool b; > + b = v[i] = true; > + b = v[i] = v[i+1]; > + > + if (v[i] = 42) { } // { dg-message "parentheses" } > + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } > + > + if ((v[i] = 42)) { } > + if ((v[i] = v[i+1])) { } > +} > + > +template<class> > +void ft(std::vector<bool> v, int i) { > + bool b; > + b = v[i] = true; > + b = v[i] = v[i+1]; > + > + if (v[i] = 42) { } // { dg-message "parentheses" } > + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } > + > + if ((v[i] = 42)) { } > + if ((v[i] = v[i+1])) { } > +}
On Wed, 20 Dec 2023, Jason Merrill wrote: > On 12/20/23 17:07, Patrick Palka wrote: > > Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this > > look OK for trunk if successful? > > > > -- >8 -- > > > > Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably > > warns on the idiom > > > > Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used > > as truth value [-Wparentheses] > > 9 | b = v[i] = true; > > | ^~~~ > > > > where v has type std::vector<bool>. That commit intended to only extend > > the existing diagnostics so that they happen in a template context as > > well, but the refactoring of is_assignment_op_expr_p caused us for this > > particular -Wparentheses warning (from convert_for_assignment) to now > > consider user-defined operator= instead of just built-in operator=. And > > since std::vector<bool> is really a bitset, whose operator[] returns a > > class type with such a user-defined operator= (taking bool), we now warn. > > > > But arguably "boolish" class types should be treated like ordinary bool > > as far as the warning is concerned. To that end this patch relaxes the > > warning for such types, specifically when the (class) type can be > > (implicitly) converted to a and assigned from a bool. This should cover > > at least implementations of std::vector<bool>::reference. > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (maybe_warn_unparenthesized_assignment): Add > > 'nested_p' bool parameter. > > * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool > > parameter and set it accordingly. > > (class_type_is_boolish_cache): Define. > > (class_type_is_boolish): Define. > > (maybe_warn_unparenthesized_assignment): Add 'nested_p' > > bool parameter. Relax the warning for nested assignments > > to boolean-like class types. > > (maybe_convert_cond): Pass nested_p=false to > > maybe_warn_unparenthesized_assignment. > > * typeck.cc (convert_for_assignment): Pass nested_p=true to > > maybe_warn_unparenthesized_assignment. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/warn/Wparentheses-34.C: New test. > > --- > > gcc/cp/cp-tree.h | 2 +- > > gcc/cp/semantics.cc | 106 ++++++++++++++++++-- > > gcc/cp/typeck.cc | 2 +- > > gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ > > 4 files changed, 129 insertions(+), 12 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index 1979572c365..97065cccf3d 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -7928,7 +7928,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); > > +extern void maybe_warn_unparenthesized_assignment (tree, bool, > > tsubst_flags_t); > > extern tree cp_check_pragma_unroll (location_t, tree); > > /* in tree.cc */ > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > > index 64839b1ac87..92acd560fa4 100644 > > --- a/gcc/cp/semantics.cc > > +++ b/gcc/cp/semantics.cc > > @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) > > return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); > > } > > -/* Returns true if T corresponds to an assignment operator expression. > > */ > > +/* Returns true if T corresponds to an assignment operator expression, > > + and sets *LHS to its left-hand-side operand if so. */ > > static bool > > -is_assignment_op_expr_p (tree t) > > +is_assignment_op_expr_p (tree t, tree *lhs) > > { > > if (t == NULL_TREE) > > return false; > > @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) > > if (TREE_CODE (t) == MODIFY_EXPR > > || (TREE_CODE (t) == MODOP_EXPR > > && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) > > - return true; > > + { > > + *lhs = TREE_OPERAND (t, 0); > > + return true; > > + } > > tree call = extract_call_expr (t); > > if (call == NULL_TREE > > @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) > > return false; > > tree fndecl = cp_get_callee_fndecl_nofold (call); > > - return fndecl != NULL_TREE > > - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > > - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); > > + if (fndecl != NULL_TREE > > + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > > + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) > > + { > > + *lhs = get_nth_callarg (call, 0); > > + return true; > > + } > > + > > + return false; > > } > > +static GTY((deletable)) hash_map<tree, bool> > > *class_type_is_boolish_cache; > > + > > +/* Return true if the class type TYPE can be converted to and assigned > > + from a boolean. */ > > + > > +static bool > > +class_type_is_boolish (tree type) > > +{ > > + type = TYPE_MAIN_VARIANT (type); > > + if (!COMPLETE_TYPE_P (type)) > > + return false; > > + > > + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) > > + return *r; > > + > > + bool has_bool_conversion = false; > > + bool has_bool_assignment = false; > > + > > + tree ops = lookup_conversions (type); > > + for (; ops; ops = TREE_CHAIN (ops)) > > + { > > + tree op = TREE_VALUE (ops); > > + if (!DECL_NONCONVERTING_P (op) > > + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) > > + { > > + has_bool_conversion = true; > > + break; > > + } > > + } > > + > > + if (!has_bool_conversion) > > + { > > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, false); > > + return false; > > + } > > + > > + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, > > tf_none); > > + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) > > + { > > + op = STRIP_TEMPLATE (op); > > + if (TREE_CODE (op) != FUNCTION_DECL) > > + continue; > > + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); > > + tree parm_type = non_reference (TREE_TYPE (parm)); > > + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) > > + { > > + has_bool_assignment = true; > > + break; > > + } > > + } > > + > > + bool boolish = has_bool_conversion && has_bool_assignment; > > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); > > + return boolish; > > +} > > + > > + > > /* Maybe warn about an unparenthesized 'a = b' (appearing in a > > - boolean context where 'a == b' might have been intended). */ > > + boolean context where 'a == b' might have been intended). > > + NESTED_P is true if T appears as the RHS of another assignment. */ > > void > > -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) > > +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, > > + tsubst_flags_t complain) > > { > > t = STRIP_REFERENCE_REF (t); > > + tree lhs; > > if ((complain & tf_warning) > > && warn_parentheses > > - && is_assignment_op_expr_p (t) > > + && is_assignment_op_expr_p (t, &lhs) > > /* A parenthesized expression would've had this warning > > suppressed by finish_parenthesized_expr. */ > > && !warning_suppressed_p (t, OPT_Wparentheses)) > > { > > + /* In a = b = c, don't warn if b is a boolean-like class type. */ > > + if (nested_p) > > + { > > + /* The caller already checked this. */ > > + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); > > It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and bool-ish > type here. Can we check both in one place or the other? Sounds good, it's easy enough to check both in maybe_warn_unparenthesized_assignment. Like so? I've opted to preserve the original slightly stronger form of the check which really checks that (b = c) has bool type rather than b having bool type, to avoid introducing new warnings. - >8 -- Subject: [PATCH] c++: fix -Wparentheses with bool-like class types Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably warns on the idiom Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses] 9 | b = v[i] = true; | ^~~~ where v has type std::vector<bool>. That commit intended to only extend the existing diagnostics so that they happen in a template context as well, but the refactoring of is_assignment_op_expr_p caused us for this particular -Wparentheses warning (from convert_for_assignment) to now consider user-defined operator= instead of just built-in operator=. And since std::vector<bool> is really a bitset, whose operator[] returns a class type with such a user-defined operator= (taking bool), we now warn. But arguably "boolish" class types should be treated like ordinary bool as far as the warning is concerned. To that end this patch relaxes the warning for such types, specifically when the (class) type can be (implicitly) converted to and assigned from bool. This criterion at least covers implementations of std::vector<bool>::reference. gcc/cp/ChangeLog: * cp-tree.h (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool parameter and set it accordingly. (class_type_is_boolish_cache): Define. (class_type_is_boolish): Define. (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. Suppress the warning for nested assignments to bool and bool-like class types. (maybe_convert_cond): Pass nested_p=false to maybe_warn_unparenthesized_assignment. * typeck.cc (convert_for_assignment): Pass nested_p=true to maybe_warn_unparenthesized_assignment. Remove now redundant check for 'rhs' having bool type. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-34.C: New test. --- gcc/cp/cp-tree.h | 2 +- gcc/cp/semantics.cc | 107 ++++++++++++++++++-- gcc/cp/typeck.cc | 7 +- gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ 4 files changed, 131 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 1979572c365..97065cccf3d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7928,7 +7928,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); +extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t); extern tree cp_check_pragma_unroll (location_t, tree); /* in tree.cc */ diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 64839b1ac87..6aa1dc46665 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); } -/* Returns true if T corresponds to an assignment operator expression. */ +/* Returns true if T corresponds to an assignment operator expression, + and sets *LHS to its left-hand-side operand if so. */ static bool -is_assignment_op_expr_p (tree t) +is_assignment_op_expr_p (tree t, tree *lhs) { if (t == NULL_TREE) return false; @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) if (TREE_CODE (t) == MODIFY_EXPR || (TREE_CODE (t) == MODOP_EXPR && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) - return true; + { + *lhs = TREE_OPERAND (t, 0); + return true; + } tree call = extract_call_expr (t); if (call == NULL_TREE @@ -859,26 +863,108 @@ is_assignment_op_expr_p (tree t) return false; tree fndecl = cp_get_callee_fndecl_nofold (call); - return fndecl != NULL_TREE - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); + if (fndecl != NULL_TREE + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) + { + *lhs = get_nth_callarg (call, 0); + return true; + } + + return false; } +static GTY((deletable)) hash_map<tree, bool> *class_type_is_boolish_cache; + +/* Return true if the class type TYPE can be converted to and assigned + from bool. */ + +static bool +class_type_is_boolish (tree type) +{ + type = TYPE_MAIN_VARIANT (type); + if (!COMPLETE_TYPE_P (type)) + return false; + + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) + return *r; + + bool has_bool_assignment = false; + bool has_bool_conversion = false; + + tree ops = lookup_fnfields (type, assign_op_identifier, + /*protect=*/0, tf_none); + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) + { + op = STRIP_TEMPLATE (op); + if (TREE_CODE (op) != FUNCTION_DECL) + continue; + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); + tree parm_type = non_reference (TREE_TYPE (parm)); + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) + { + has_bool_assignment = true; + break; + } + } + + if (!has_bool_assignment) + { + hash_map_safe_put<true> (class_type_is_boolish_cache, type, false); + return false; + } + + ops = lookup_conversions (type); + for (; ops; ops = TREE_CHAIN (ops)) + { + tree op = TREE_VALUE (ops); + if (!DECL_NONCONVERTING_P (op) + && TREE_CODE (DECL_CONV_FN_TYPE (op)) == BOOLEAN_TYPE) + { + has_bool_conversion = true; + break; + } + } + + bool boolish = has_bool_assignment && has_bool_conversion; + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); + return boolish; +} + + /* Maybe warn about an unparenthesized 'a = b' (appearing in a - boolean context where 'a == b' might have been intended). */ + boolean context where 'a == b' might have been intended). + NESTED_P is true if T appears as the RHS of another assignment. */ void -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, + tsubst_flags_t complain) { t = STRIP_REFERENCE_REF (t); + tree lhs; if ((complain & tf_warning) && warn_parentheses - && is_assignment_op_expr_p (t) + && is_assignment_op_expr_p (t, &lhs) /* A parenthesized expression would've had this warning suppressed by finish_parenthesized_expr. */ && !warning_suppressed_p (t, OPT_Wparentheses)) { + if (nested_p) + { + /* In ... = a = b, don't warn if a is a bool or bool-like class. */ + if (TREE_CODE (TREE_TYPE (t)) == BOOLEAN_TYPE) + return; + + if (TREE_CODE (lhs) == ADDR_EXPR) + lhs = TREE_OPERAND (lhs, 0); + + tree lhs_type = non_reference (TREE_TYPE (lhs)); + if (CLASS_TYPE_P (lhs_type) + && class_type_is_boolish (lhs_type)) + return; + } + warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses, "suggest parentheses around assignment used as truth value"); suppress_warning (t, OPT_Wparentheses); @@ -903,7 +989,8 @@ maybe_convert_cond (tree cond) if (warn_sequence_point && !processing_template_decl) verify_sequence_points (cond); - maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error); + maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, + tf_warning_or_error); /* Do the conversion. */ cond = convert_from_reference (cond); diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index a6e2f4ee7da..2f94dcb7938 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -10377,11 +10377,8 @@ convert_for_assignment (tree type, tree rhs, } } - /* If -Wparentheses, warn about a = b = c when a has type bool and b - does not. */ - if (TREE_CODE (type) == BOOLEAN_TYPE - && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE) - maybe_warn_unparenthesized_assignment (rhs, complain); + if (TREE_CODE (type) == BOOLEAN_TYPE) + maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain); if (complain & tf_warning) warn_for_address_of_packed_member (type, rhs); diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C new file mode 100644 index 00000000000..2100c8a193d --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C @@ -0,0 +1,31 @@ +// Verify our -Wparentheses warning handles "boolish" class types +// such as std::vector<bool>'s reference type the same as ordinary +// bool. +// { dg-additional-options "-Wparentheses" } + +#include <vector> + +void f(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +} + +template<class> +void ft(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +}
On 12/20/23 17:54, Patrick Palka wrote: > On Wed, 20 Dec 2023, Jason Merrill wrote: > >> On 12/20/23 17:07, Patrick Palka wrote: >>> Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this >>> look OK for trunk if successful? >>> >>> -- >8 -- >>> >>> Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably >>> warns on the idiom >>> >>> Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used >>> as truth value [-Wparentheses] >>> 9 | b = v[i] = true; >>> | ^~~~ >>> >>> where v has type std::vector<bool>. That commit intended to only extend >>> the existing diagnostics so that they happen in a template context as >>> well, but the refactoring of is_assignment_op_expr_p caused us for this >>> particular -Wparentheses warning (from convert_for_assignment) to now >>> consider user-defined operator= instead of just built-in operator=. And >>> since std::vector<bool> is really a bitset, whose operator[] returns a >>> class type with such a user-defined operator= (taking bool), we now warn. >>> >>> But arguably "boolish" class types should be treated like ordinary bool >>> as far as the warning is concerned. To that end this patch relaxes the >>> warning for such types, specifically when the (class) type can be >>> (implicitly) converted to a and assigned from a bool. This should cover >>> at least implementations of std::vector<bool>::reference. >>> >>> gcc/cp/ChangeLog: >>> >>> * cp-tree.h (maybe_warn_unparenthesized_assignment): Add >>> 'nested_p' bool parameter. >>> * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool >>> parameter and set it accordingly. >>> (class_type_is_boolish_cache): Define. >>> (class_type_is_boolish): Define. >>> (maybe_warn_unparenthesized_assignment): Add 'nested_p' >>> bool parameter. Relax the warning for nested assignments >>> to boolean-like class types. >>> (maybe_convert_cond): Pass nested_p=false to >>> maybe_warn_unparenthesized_assignment. >>> * typeck.cc (convert_for_assignment): Pass nested_p=true to >>> maybe_warn_unparenthesized_assignment. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/warn/Wparentheses-34.C: New test. >>> --- >>> gcc/cp/cp-tree.h | 2 +- >>> gcc/cp/semantics.cc | 106 ++++++++++++++++++-- >>> gcc/cp/typeck.cc | 2 +- >>> gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ >>> 4 files changed, 129 insertions(+), 12 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C >>> >>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >>> index 1979572c365..97065cccf3d 100644 >>> --- a/gcc/cp/cp-tree.h >>> +++ b/gcc/cp/cp-tree.h >>> @@ -7928,7 +7928,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); >>> +extern void maybe_warn_unparenthesized_assignment (tree, bool, >>> tsubst_flags_t); >>> extern tree cp_check_pragma_unroll (location_t, tree); >>> /* in tree.cc */ >>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc >>> index 64839b1ac87..92acd560fa4 100644 >>> --- a/gcc/cp/semantics.cc >>> +++ b/gcc/cp/semantics.cc >>> @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) >>> return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); >>> } >>> -/* Returns true if T corresponds to an assignment operator expression. >>> */ >>> +/* Returns true if T corresponds to an assignment operator expression, >>> + and sets *LHS to its left-hand-side operand if so. */ >>> static bool >>> -is_assignment_op_expr_p (tree t) >>> +is_assignment_op_expr_p (tree t, tree *lhs) >>> { >>> if (t == NULL_TREE) >>> return false; >>> @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) >>> if (TREE_CODE (t) == MODIFY_EXPR >>> || (TREE_CODE (t) == MODOP_EXPR >>> && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) >>> - return true; >>> + { >>> + *lhs = TREE_OPERAND (t, 0); >>> + return true; >>> + } >>> tree call = extract_call_expr (t); >>> if (call == NULL_TREE >>> @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) >>> return false; >>> tree fndecl = cp_get_callee_fndecl_nofold (call); >>> - return fndecl != NULL_TREE >>> - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) >>> - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); >>> + if (fndecl != NULL_TREE >>> + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) >>> + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) >>> + { >>> + *lhs = get_nth_callarg (call, 0); >>> + return true; >>> + } >>> + >>> + return false; >>> } >>> +static GTY((deletable)) hash_map<tree, bool> >>> *class_type_is_boolish_cache; >>> + >>> +/* Return true if the class type TYPE can be converted to and assigned >>> + from a boolean. */ >>> + >>> +static bool >>> +class_type_is_boolish (tree type) >>> +{ >>> + type = TYPE_MAIN_VARIANT (type); >>> + if (!COMPLETE_TYPE_P (type)) >>> + return false; >>> + >>> + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) >>> + return *r; >>> + >>> + bool has_bool_conversion = false; >>> + bool has_bool_assignment = false; >>> + >>> + tree ops = lookup_conversions (type); >>> + for (; ops; ops = TREE_CHAIN (ops)) >>> + { >>> + tree op = TREE_VALUE (ops); >>> + if (!DECL_NONCONVERTING_P (op) >>> + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) >>> + { >>> + has_bool_conversion = true; >>> + break; >>> + } >>> + } >>> + >>> + if (!has_bool_conversion) >>> + { >>> + hash_map_safe_put<true> (class_type_is_boolish_cache, type, false); >>> + return false; >>> + } >>> + >>> + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, >>> tf_none); >>> + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) >>> + { >>> + op = STRIP_TEMPLATE (op); >>> + if (TREE_CODE (op) != FUNCTION_DECL) >>> + continue; >>> + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); >>> + tree parm_type = non_reference (TREE_TYPE (parm)); >>> + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) >>> + { >>> + has_bool_assignment = true; >>> + break; >>> + } >>> + } >>> + >>> + bool boolish = has_bool_conversion && has_bool_assignment; >>> + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); >>> + return boolish; >>> +} >>> + >>> + >>> /* Maybe warn about an unparenthesized 'a = b' (appearing in a >>> - boolean context where 'a == b' might have been intended). */ >>> + boolean context where 'a == b' might have been intended). >>> + NESTED_P is true if T appears as the RHS of another assignment. */ >>> void >>> -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) >>> +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, >>> + tsubst_flags_t complain) >>> { >>> t = STRIP_REFERENCE_REF (t); >>> + tree lhs; >>> if ((complain & tf_warning) >>> && warn_parentheses >>> - && is_assignment_op_expr_p (t) >>> + && is_assignment_op_expr_p (t, &lhs) >>> /* A parenthesized expression would've had this warning >>> suppressed by finish_parenthesized_expr. */ >>> && !warning_suppressed_p (t, OPT_Wparentheses)) >>> { >>> + /* In a = b = c, don't warn if b is a boolean-like class type. */ >>> + if (nested_p) >>> + { >>> + /* The caller already checked this. */ >>> + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); >> >> It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and bool-ish >> type here. Can we check both in one place or the other? > > Sounds good, it's easy enough to check both in > maybe_warn_unparenthesized_assignment. Like so? I've opted to preserve the > original slightly stronger form of the check which really checks that > (b = c) has bool type rather than b having bool type, to avoid > introducing new warnings. Do we not want the same for the class case? Jason
On Wed, 20 Dec 2023, Jason Merrill wrote: > On 12/20/23 17:54, Patrick Palka wrote: > > On Wed, 20 Dec 2023, Jason Merrill wrote: > > > > > On 12/20/23 17:07, Patrick Palka wrote: > > > > Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this > > > > look OK for trunk if successful? > > > > > > > > -- >8 -- > > > > > > > > Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably > > > > warns on the idiom > > > > > > > > Wparentheses-34.C:9:14: warning: suggest parentheses around assignment > > > > used > > > > as truth value [-Wparentheses] > > > > 9 | b = v[i] = true; > > > > | ^~~~ > > > > > > > > where v has type std::vector<bool>. That commit intended to only extend > > > > the existing diagnostics so that they happen in a template context as > > > > well, but the refactoring of is_assignment_op_expr_p caused us for this > > > > particular -Wparentheses warning (from convert_for_assignment) to now > > > > consider user-defined operator= instead of just built-in operator=. And > > > > since std::vector<bool> is really a bitset, whose operator[] returns a > > > > class type with such a user-defined operator= (taking bool), we now > > > > warn. > > > > > > > > But arguably "boolish" class types should be treated like ordinary bool > > > > as far as the warning is concerned. To that end this patch relaxes the > > > > warning for such types, specifically when the (class) type can be > > > > (implicitly) converted to a and assigned from a bool. This should cover > > > > at least implementations of std::vector<bool>::reference. > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * cp-tree.h (maybe_warn_unparenthesized_assignment): Add > > > > 'nested_p' bool parameter. > > > > * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool > > > > parameter and set it accordingly. > > > > (class_type_is_boolish_cache): Define. > > > > (class_type_is_boolish): Define. > > > > (maybe_warn_unparenthesized_assignment): Add 'nested_p' > > > > bool parameter. Relax the warning for nested assignments > > > > to boolean-like class types. > > > > (maybe_convert_cond): Pass nested_p=false to > > > > maybe_warn_unparenthesized_assignment. > > > > * typeck.cc (convert_for_assignment): Pass nested_p=true to > > > > maybe_warn_unparenthesized_assignment. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/warn/Wparentheses-34.C: New test. > > > > --- > > > > gcc/cp/cp-tree.h | 2 +- > > > > gcc/cp/semantics.cc | 106 > > > > ++++++++++++++++++-- > > > > gcc/cp/typeck.cc | 2 +- > > > > gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ > > > > 4 files changed, 129 insertions(+), 12 deletions(-) > > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C > > > > > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > > > index 1979572c365..97065cccf3d 100644 > > > > --- a/gcc/cp/cp-tree.h > > > > +++ b/gcc/cp/cp-tree.h > > > > @@ -7928,7 +7928,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); > > > > +extern void maybe_warn_unparenthesized_assignment (tree, bool, > > > > tsubst_flags_t); > > > > extern tree cp_check_pragma_unroll (location_t, tree); > > > > /* in tree.cc */ > > > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > > > > index 64839b1ac87..92acd560fa4 100644 > > > > --- a/gcc/cp/semantics.cc > > > > +++ b/gcc/cp/semantics.cc > > > > @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) > > > > return add_stmt (build_stmt (input_location, GOTO_EXPR, > > > > destination)); > > > > } > > > > -/* Returns true if T corresponds to an assignment operator > > > > expression. > > > > */ > > > > +/* Returns true if T corresponds to an assignment operator expression, > > > > + and sets *LHS to its left-hand-side operand if so. */ > > > > static bool > > > > -is_assignment_op_expr_p (tree t) > > > > +is_assignment_op_expr_p (tree t, tree *lhs) > > > > { > > > > if (t == NULL_TREE) > > > > return false; > > > > @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) > > > > if (TREE_CODE (t) == MODIFY_EXPR > > > > || (TREE_CODE (t) == MODOP_EXPR > > > > && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) > > > > - return true; > > > > + { > > > > + *lhs = TREE_OPERAND (t, 0); > > > > + return true; > > > > + } > > > > tree call = extract_call_expr (t); > > > > if (call == NULL_TREE > > > > @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) > > > > return false; > > > > tree fndecl = cp_get_callee_fndecl_nofold (call); > > > > - return fndecl != NULL_TREE > > > > - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > > > > - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); > > > > + if (fndecl != NULL_TREE > > > > + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) > > > > + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) > > > > + { > > > > + *lhs = get_nth_callarg (call, 0); > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > } > > > > +static GTY((deletable)) hash_map<tree, bool> > > > > *class_type_is_boolish_cache; > > > > + > > > > +/* Return true if the class type TYPE can be converted to and assigned > > > > + from a boolean. */ > > > > + > > > > +static bool > > > > +class_type_is_boolish (tree type) > > > > +{ > > > > + type = TYPE_MAIN_VARIANT (type); > > > > + if (!COMPLETE_TYPE_P (type)) > > > > + return false; > > > > + > > > > + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) > > > > + return *r; > > > > + > > > > + bool has_bool_conversion = false; > > > > + bool has_bool_assignment = false; > > > > + > > > > + tree ops = lookup_conversions (type); > > > > + for (; ops; ops = TREE_CHAIN (ops)) > > > > + { > > > > + tree op = TREE_VALUE (ops); > > > > + if (!DECL_NONCONVERTING_P (op) > > > > + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) > > > > + { > > > > + has_bool_conversion = true; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (!has_bool_conversion) > > > > + { > > > > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, > > > > false); > > > > + return false; > > > > + } > > > > + > > > > + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, > > > > tf_none); > > > > + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) > > > > + { > > > > + op = STRIP_TEMPLATE (op); > > > > + if (TREE_CODE (op) != FUNCTION_DECL) > > > > + continue; > > > > + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); > > > > + tree parm_type = non_reference (TREE_TYPE (parm)); > > > > + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) > > > > + { > > > > + has_bool_assignment = true; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + bool boolish = has_bool_conversion && has_bool_assignment; > > > > + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); > > > > + return boolish; > > > > +} > > > > + > > > > + > > > > /* Maybe warn about an unparenthesized 'a = b' (appearing in a > > > > - boolean context where 'a == b' might have been intended). */ > > > > + boolean context where 'a == b' might have been intended). > > > > + NESTED_P is true if T appears as the RHS of another assignment. */ > > > > void > > > > -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) > > > > +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, > > > > + tsubst_flags_t complain) > > > > { > > > > t = STRIP_REFERENCE_REF (t); > > > > + tree lhs; > > > > if ((complain & tf_warning) > > > > && warn_parentheses > > > > - && is_assignment_op_expr_p (t) > > > > + && is_assignment_op_expr_p (t, &lhs) > > > > /* A parenthesized expression would've had this warning > > > > suppressed by finish_parenthesized_expr. */ > > > > && !warning_suppressed_p (t, OPT_Wparentheses)) > > > > { > > > > + /* In a = b = c, don't warn if b is a boolean-like class type. > > > > */ > > > > + if (nested_p) > > > > + { > > > > + /* The caller already checked this. */ > > > > + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); > > > > > > It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and > > > bool-ish > > > type here. Can we check both in one place or the other? > > > > Sounds good, it's easy enough to check both in > > maybe_warn_unparenthesized_assignment. Like so? I've opted to preserve the > > original slightly stronger form of the check which really checks that > > (b = c) has bool type rather than b having bool type, to avoid > > introducing new warnings. > > Do we not want the same for the class case? IIUC whether we check the type of (b = c) or just that of b only makes a difference for weird user-defined operator= that returns something other than *this... and for the kind of bool-like classes we wish to identify, that won't be the case. And checking the type of (b = c) certainly is simpler and also more consistent, so we might as well go with that. But we should still perform the check in maybe_warn_unparenthesized_assignment rather than its caller since boolish_class_type_p is a relatively expensive predicate so we should check it last. Like so? Bootstrap and regtest running on x86_64-pc-linux-gnu. -- >8 -- gcc/cp/ChangeLog: * cp-tree.h (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. * semantics.cc (boolish_class_type_p_cache): Define. (boolish_class_type_p): Define. (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. Suppress the warning for nested assignments to bool and bool-like class types. (maybe_convert_cond): Pass nested_p=false to maybe_warn_unparenthesized_assignment. * typeck.cc (convert_for_assignment): Pass nested_p=true to maybe_warn_unparenthesized_assignment. Remove now redundant check for 'rhs' having bool type. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-34.C: New test. --- gcc/cp/cp-tree.h | 2 +- gcc/cp/semantics.cc | 71 +++++++++++++++++++-- gcc/cp/typeck.cc | 7 +- gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 +++++++++ 4 files changed, 101 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 1979572c365..97065cccf3d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7928,7 +7928,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); +extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t); extern tree cp_check_pragma_unroll (location_t, tree); /* in tree.cc */ diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 64839b1ac87..46b828d1483 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -864,12 +864,70 @@ is_assignment_op_expr_p (tree t) && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); } +static GTY((deletable)) hash_map<tree, bool> *boolish_class_type_p_cache; + +/* Return true if the class type TYPE can be converted to and assigned + from bool. */ + +static bool +boolish_class_type_p (tree type) +{ + type = TYPE_MAIN_VARIANT (type); + if (!CLASS_TYPE_P (type) || !COMPLETE_TYPE_P (type)) + return false; + + if (bool *r = hash_map_safe_get (boolish_class_type_p_cache, type)) + return *r; + + bool has_bool_assignment = false; + bool has_bool_conversion = false; + + tree ops = lookup_fnfields (type, assign_op_identifier, + /*protect=*/0, tf_none); + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) + { + op = STRIP_TEMPLATE (op); + if (TREE_CODE (op) != FUNCTION_DECL) + continue; + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); + tree parm_type = non_reference (TREE_TYPE (parm)); + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) + { + has_bool_assignment = true; + break; + } + } + + if (has_bool_assignment) + { + ops = lookup_conversions (type); + for (; ops; ops = TREE_CHAIN (ops)) + { + tree op = TREE_VALUE (ops); + if (!DECL_NONCONVERTING_P (op) + && TREE_CODE (DECL_CONV_FN_TYPE (op)) == BOOLEAN_TYPE) + { + has_bool_conversion = true; + break; + } + } + } + + bool boolish = has_bool_assignment && has_bool_conversion; + hash_map_safe_put<true> (boolish_class_type_p_cache, type, boolish); + return boolish; +} + + /* Maybe warn about an unparenthesized 'a = b' (appearing in a - boolean context where 'a == b' might have been intended). */ + boolean context where 'a == b' might have been intended). + NESTED_P is true if T appears as the RHS of another assignment. */ void -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, + tsubst_flags_t complain) { + tree type = TREE_TYPE (t); t = STRIP_REFERENCE_REF (t); if ((complain & tf_warning) @@ -877,7 +935,11 @@ maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) && is_assignment_op_expr_p (t) /* A parenthesized expression would've had this warning suppressed by finish_parenthesized_expr. */ - && !warning_suppressed_p (t, OPT_Wparentheses)) + && !warning_suppressed_p (t, OPT_Wparentheses) + /* In ... = a = b, don't warn if a has type bool or bool-like class. */ + && (!nested_p + || (TREE_CODE (type) != BOOLEAN_TYPE + && !boolish_class_type_p (type)))) { warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses, "suggest parentheses around assignment used as truth value"); @@ -903,7 +965,8 @@ maybe_convert_cond (tree cond) if (warn_sequence_point && !processing_template_decl) verify_sequence_points (cond); - maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error); + maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, + tf_warning_or_error); /* Do the conversion. */ cond = convert_from_reference (cond); diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index a6e2f4ee7da..2f94dcb7938 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -10377,11 +10377,8 @@ convert_for_assignment (tree type, tree rhs, } } - /* If -Wparentheses, warn about a = b = c when a has type bool and b - does not. */ - if (TREE_CODE (type) == BOOLEAN_TYPE - && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE) - maybe_warn_unparenthesized_assignment (rhs, complain); + if (TREE_CODE (type) == BOOLEAN_TYPE) + maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain); if (complain & tf_warning) warn_for_address_of_packed_member (type, rhs); diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C new file mode 100644 index 00000000000..2100c8a193d --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C @@ -0,0 +1,31 @@ +// Verify our -Wparentheses warning handles "boolish" class types +// such as std::vector<bool>'s reference type the same as ordinary +// bool. +// { dg-additional-options "-Wparentheses" } + +#include <vector> + +void f(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +} + +template<class> +void ft(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +}
On 12/20/23 20:01, Patrick Palka wrote: > On Wed, 20 Dec 2023, Jason Merrill wrote: > >> On 12/20/23 17:54, Patrick Palka wrote: >>> On Wed, 20 Dec 2023, Jason Merrill wrote: >>> >>>> On 12/20/23 17:07, Patrick Palka wrote: >>>>> Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this >>>>> look OK for trunk if successful? >>>>> >>>>> -- >8 -- >>>>> >>>>> Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably >>>>> warns on the idiom >>>>> >>>>> Wparentheses-34.C:9:14: warning: suggest parentheses around assignment >>>>> used >>>>> as truth value [-Wparentheses] >>>>> 9 | b = v[i] = true; >>>>> | ^~~~ >>>>> >>>>> where v has type std::vector<bool>. That commit intended to only extend >>>>> the existing diagnostics so that they happen in a template context as >>>>> well, but the refactoring of is_assignment_op_expr_p caused us for this >>>>> particular -Wparentheses warning (from convert_for_assignment) to now >>>>> consider user-defined operator= instead of just built-in operator=. And >>>>> since std::vector<bool> is really a bitset, whose operator[] returns a >>>>> class type with such a user-defined operator= (taking bool), we now >>>>> warn. >>>>> >>>>> But arguably "boolish" class types should be treated like ordinary bool >>>>> as far as the warning is concerned. To that end this patch relaxes the >>>>> warning for such types, specifically when the (class) type can be >>>>> (implicitly) converted to a and assigned from a bool. This should cover >>>>> at least implementations of std::vector<bool>::reference. >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * cp-tree.h (maybe_warn_unparenthesized_assignment): Add >>>>> 'nested_p' bool parameter. >>>>> * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool >>>>> parameter and set it accordingly. >>>>> (class_type_is_boolish_cache): Define. >>>>> (class_type_is_boolish): Define. >>>>> (maybe_warn_unparenthesized_assignment): Add 'nested_p' >>>>> bool parameter. Relax the warning for nested assignments >>>>> to boolean-like class types. >>>>> (maybe_convert_cond): Pass nested_p=false to >>>>> maybe_warn_unparenthesized_assignment. >>>>> * typeck.cc (convert_for_assignment): Pass nested_p=true to >>>>> maybe_warn_unparenthesized_assignment. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/warn/Wparentheses-34.C: New test. >>>>> --- >>>>> gcc/cp/cp-tree.h | 2 +- >>>>> gcc/cp/semantics.cc | 106 >>>>> ++++++++++++++++++-- >>>>> gcc/cp/typeck.cc | 2 +- >>>>> gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ >>>>> 4 files changed, 129 insertions(+), 12 deletions(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C >>>>> >>>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >>>>> index 1979572c365..97065cccf3d 100644 >>>>> --- a/gcc/cp/cp-tree.h >>>>> +++ b/gcc/cp/cp-tree.h >>>>> @@ -7928,7 +7928,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); >>>>> +extern void maybe_warn_unparenthesized_assignment (tree, bool, >>>>> tsubst_flags_t); >>>>> extern tree cp_check_pragma_unroll (location_t, tree); >>>>> /* in tree.cc */ >>>>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc >>>>> index 64839b1ac87..92acd560fa4 100644 >>>>> --- a/gcc/cp/semantics.cc >>>>> +++ b/gcc/cp/semantics.cc >>>>> @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) >>>>> return add_stmt (build_stmt (input_location, GOTO_EXPR, >>>>> destination)); >>>>> } >>>>> -/* Returns true if T corresponds to an assignment operator >>>>> expression. >>>>> */ >>>>> +/* Returns true if T corresponds to an assignment operator expression, >>>>> + and sets *LHS to its left-hand-side operand if so. */ >>>>> static bool >>>>> -is_assignment_op_expr_p (tree t) >>>>> +is_assignment_op_expr_p (tree t, tree *lhs) >>>>> { >>>>> if (t == NULL_TREE) >>>>> return false; >>>>> @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) >>>>> if (TREE_CODE (t) == MODIFY_EXPR >>>>> || (TREE_CODE (t) == MODOP_EXPR >>>>> && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) >>>>> - return true; >>>>> + { >>>>> + *lhs = TREE_OPERAND (t, 0); >>>>> + return true; >>>>> + } >>>>> tree call = extract_call_expr (t); >>>>> if (call == NULL_TREE >>>>> @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) >>>>> return false; >>>>> tree fndecl = cp_get_callee_fndecl_nofold (call); >>>>> - return fndecl != NULL_TREE >>>>> - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) >>>>> - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); >>>>> + if (fndecl != NULL_TREE >>>>> + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) >>>>> + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) >>>>> + { >>>>> + *lhs = get_nth_callarg (call, 0); >>>>> + return true; >>>>> + } >>>>> + >>>>> + return false; >>>>> } >>>>> +static GTY((deletable)) hash_map<tree, bool> >>>>> *class_type_is_boolish_cache; >>>>> + >>>>> +/* Return true if the class type TYPE can be converted to and assigned >>>>> + from a boolean. */ >>>>> + >>>>> +static bool >>>>> +class_type_is_boolish (tree type) >>>>> +{ >>>>> + type = TYPE_MAIN_VARIANT (type); >>>>> + if (!COMPLETE_TYPE_P (type)) >>>>> + return false; >>>>> + >>>>> + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) >>>>> + return *r; >>>>> + >>>>> + bool has_bool_conversion = false; >>>>> + bool has_bool_assignment = false; >>>>> + >>>>> + tree ops = lookup_conversions (type); >>>>> + for (; ops; ops = TREE_CHAIN (ops)) >>>>> + { >>>>> + tree op = TREE_VALUE (ops); >>>>> + if (!DECL_NONCONVERTING_P (op) >>>>> + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) >>>>> + { >>>>> + has_bool_conversion = true; >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + if (!has_bool_conversion) >>>>> + { >>>>> + hash_map_safe_put<true> (class_type_is_boolish_cache, type, >>>>> false); >>>>> + return false; >>>>> + } >>>>> + >>>>> + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, >>>>> tf_none); >>>>> + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) >>>>> + { >>>>> + op = STRIP_TEMPLATE (op); >>>>> + if (TREE_CODE (op) != FUNCTION_DECL) >>>>> + continue; >>>>> + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); >>>>> + tree parm_type = non_reference (TREE_TYPE (parm)); >>>>> + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) >>>>> + { >>>>> + has_bool_assignment = true; >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + bool boolish = has_bool_conversion && has_bool_assignment; >>>>> + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); >>>>> + return boolish; >>>>> +} >>>>> + >>>>> + >>>>> /* Maybe warn about an unparenthesized 'a = b' (appearing in a >>>>> - boolean context where 'a == b' might have been intended). */ >>>>> + boolean context where 'a == b' might have been intended). >>>>> + NESTED_P is true if T appears as the RHS of another assignment. */ >>>>> void >>>>> -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) >>>>> +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, >>>>> + tsubst_flags_t complain) >>>>> { >>>>> t = STRIP_REFERENCE_REF (t); >>>>> + tree lhs; >>>>> if ((complain & tf_warning) >>>>> && warn_parentheses >>>>> - && is_assignment_op_expr_p (t) >>>>> + && is_assignment_op_expr_p (t, &lhs) >>>>> /* A parenthesized expression would've had this warning >>>>> suppressed by finish_parenthesized_expr. */ >>>>> && !warning_suppressed_p (t, OPT_Wparentheses)) >>>>> { >>>>> + /* In a = b = c, don't warn if b is a boolean-like class type. >>>>> */ >>>>> + if (nested_p) >>>>> + { >>>>> + /* The caller already checked this. */ >>>>> + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); >>>> >>>> It seems awkward to check BOOLEAN_TYPE in convert_for_assignment and >>>> bool-ish >>>> type here. Can we check both in one place or the other? >>> >>> Sounds good, it's easy enough to check both in >>> maybe_warn_unparenthesized_assignment. Like so? I've opted to preserve the >>> original slightly stronger form of the check which really checks that >>> (b = c) has bool type rather than b having bool type, to avoid >>> introducing new warnings. >> >> Do we not want the same for the class case? > > IIUC whether we check the type of (b = c) or just that of b only makes a > difference for weird user-defined operator= that returns something other > than *this... and for the kind of bool-like classes we wish to identify, > that won't be the case. > > And checking the type of (b = c) certainly is simpler and also more > consistent, so we might as well go with that. But we should still > perform the check in maybe_warn_unparenthesized_assignment rather > than its caller since boolish_class_type_p is a relatively expensive > predicate so we should check it last. Like so? Bootstrap and > regtest running on x86_64-pc-linux-gnu. OK, thanks. > -- >8 -- > > gcc/cp/ChangeLog: > > * cp-tree.h (maybe_warn_unparenthesized_assignment): Add > 'nested_p' bool parameter. > * semantics.cc > (boolish_class_type_p_cache): Define. > (boolish_class_type_p): Define. > (maybe_warn_unparenthesized_assignment): Add 'nested_p' > bool parameter. Suppress the warning for nested assignments > to bool and bool-like class types. > (maybe_convert_cond): Pass nested_p=false to > maybe_warn_unparenthesized_assignment. > * typeck.cc (convert_for_assignment): Pass nested_p=true to > maybe_warn_unparenthesized_assignment. Remove now redundant > check for 'rhs' having bool type. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wparentheses-34.C: New test. > --- > gcc/cp/cp-tree.h | 2 +- > gcc/cp/semantics.cc | 71 +++++++++++++++++++-- > gcc/cp/typeck.cc | 7 +- > gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 +++++++++ > 4 files changed, 101 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 1979572c365..97065cccf3d 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7928,7 +7928,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); > +extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t); > extern tree cp_check_pragma_unroll (location_t, tree); > > /* in tree.cc */ > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index 64839b1ac87..46b828d1483 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -864,12 +864,70 @@ is_assignment_op_expr_p (tree t) > && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); > } > > +static GTY((deletable)) hash_map<tree, bool> *boolish_class_type_p_cache; > + > +/* Return true if the class type TYPE can be converted to and assigned > + from bool. */ > + > +static bool > +boolish_class_type_p (tree type) > +{ > + type = TYPE_MAIN_VARIANT (type); > + if (!CLASS_TYPE_P (type) || !COMPLETE_TYPE_P (type)) > + return false; > + > + if (bool *r = hash_map_safe_get (boolish_class_type_p_cache, type)) > + return *r; > + > + bool has_bool_assignment = false; > + bool has_bool_conversion = false; > + > + tree ops = lookup_fnfields (type, assign_op_identifier, > + /*protect=*/0, tf_none); > + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) > + { > + op = STRIP_TEMPLATE (op); > + if (TREE_CODE (op) != FUNCTION_DECL) > + continue; > + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); > + tree parm_type = non_reference (TREE_TYPE (parm)); > + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) > + { > + has_bool_assignment = true; > + break; > + } > + } > + > + if (has_bool_assignment) > + { > + ops = lookup_conversions (type); > + for (; ops; ops = TREE_CHAIN (ops)) > + { > + tree op = TREE_VALUE (ops); > + if (!DECL_NONCONVERTING_P (op) > + && TREE_CODE (DECL_CONV_FN_TYPE (op)) == BOOLEAN_TYPE) > + { > + has_bool_conversion = true; > + break; > + } > + } > + } > + > + bool boolish = has_bool_assignment && has_bool_conversion; > + hash_map_safe_put<true> (boolish_class_type_p_cache, type, boolish); > + return boolish; > +} > + > + > /* Maybe warn about an unparenthesized 'a = b' (appearing in a > - boolean context where 'a == b' might have been intended). */ > + boolean context where 'a == b' might have been intended). > + NESTED_P is true if T appears as the RHS of another assignment. */ > > void > -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) > +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, > + tsubst_flags_t complain) > { > + tree type = TREE_TYPE (t); > t = STRIP_REFERENCE_REF (t); > > if ((complain & tf_warning) > @@ -877,7 +935,11 @@ maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) > && is_assignment_op_expr_p (t) > /* A parenthesized expression would've had this warning > suppressed by finish_parenthesized_expr. */ > - && !warning_suppressed_p (t, OPT_Wparentheses)) > + && !warning_suppressed_p (t, OPT_Wparentheses) > + /* In ... = a = b, don't warn if a has type bool or bool-like class. */ > + && (!nested_p > + || (TREE_CODE (type) != BOOLEAN_TYPE > + && !boolish_class_type_p (type)))) > { > warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses, > "suggest parentheses around assignment used as truth value"); > @@ -903,7 +965,8 @@ maybe_convert_cond (tree cond) > if (warn_sequence_point && !processing_template_decl) > verify_sequence_points (cond); > > - maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error); > + maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, > + tf_warning_or_error); > > /* Do the conversion. */ > cond = convert_from_reference (cond); > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index a6e2f4ee7da..2f94dcb7938 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10377,11 +10377,8 @@ convert_for_assignment (tree type, tree rhs, > } > } > > - /* If -Wparentheses, warn about a = b = c when a has type bool and b > - does not. */ > - if (TREE_CODE (type) == BOOLEAN_TYPE > - && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE) > - maybe_warn_unparenthesized_assignment (rhs, complain); > + if (TREE_CODE (type) == BOOLEAN_TYPE) > + maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain); > > if (complain & tf_warning) > warn_for_address_of_packed_member (type, rhs); > diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C > new file mode 100644 > index 00000000000..2100c8a193d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C > @@ -0,0 +1,31 @@ > +// Verify our -Wparentheses warning handles "boolish" class types > +// such as std::vector<bool>'s reference type the same as ordinary > +// bool. > +// { dg-additional-options "-Wparentheses" } > + > +#include <vector> > + > +void f(std::vector<bool> v, int i) { > + bool b; > + b = v[i] = true; > + b = v[i] = v[i+1]; > + > + if (v[i] = 42) { } // { dg-message "parentheses" } > + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } > + > + if ((v[i] = 42)) { } > + if ((v[i] = v[i+1])) { } > +} > + > +template<class> > +void ft(std::vector<bool> v, int i) { > + bool b; > + b = v[i] = true; > + b = v[i] = v[i+1]; > + > + if (v[i] = 42) { } // { dg-message "parentheses" } > + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } > + > + if ((v[i] = 42)) { } > + if ((v[i] = v[i+1])) { } > +}
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 1979572c365..97065cccf3d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7928,7 +7928,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); +extern void maybe_warn_unparenthesized_assignment (tree, bool, tsubst_flags_t); extern tree cp_check_pragma_unroll (location_t, tree); /* in tree.cc */ diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index 64839b1ac87..92acd560fa4 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -839,10 +839,11 @@ finish_goto_stmt (tree destination) return add_stmt (build_stmt (input_location, GOTO_EXPR, destination)); } -/* Returns true if T corresponds to an assignment operator expression. */ +/* Returns true if T corresponds to an assignment operator expression, + and sets *LHS to its left-hand-side operand if so. */ static bool -is_assignment_op_expr_p (tree t) +is_assignment_op_expr_p (tree t, tree *lhs) { if (t == NULL_TREE) return false; @@ -850,7 +851,10 @@ is_assignment_op_expr_p (tree t) if (TREE_CODE (t) == MODIFY_EXPR || (TREE_CODE (t) == MODOP_EXPR && TREE_CODE (TREE_OPERAND (t, 1)) == NOP_EXPR)) - return true; + { + *lhs = TREE_OPERAND (t, 0); + return true; + } tree call = extract_call_expr (t); if (call == NULL_TREE @@ -859,26 +863,107 @@ is_assignment_op_expr_p (tree t) return false; tree fndecl = cp_get_callee_fndecl_nofold (call); - return fndecl != NULL_TREE - && DECL_ASSIGNMENT_OPERATOR_P (fndecl) - && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR); + if (fndecl != NULL_TREE + && DECL_ASSIGNMENT_OPERATOR_P (fndecl) + && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)) + { + *lhs = get_nth_callarg (call, 0); + return true; + } + + return false; } +static GTY((deletable)) hash_map<tree, bool> *class_type_is_boolish_cache; + +/* Return true if the class type TYPE can be converted to and assigned + from a boolean. */ + +static bool +class_type_is_boolish (tree type) +{ + type = TYPE_MAIN_VARIANT (type); + if (!COMPLETE_TYPE_P (type)) + return false; + + if (bool *r = hash_map_safe_get (class_type_is_boolish_cache, type)) + return *r; + + bool has_bool_conversion = false; + bool has_bool_assignment = false; + + tree ops = lookup_conversions (type); + for (; ops; ops = TREE_CHAIN (ops)) + { + tree op = TREE_VALUE (ops); + if (!DECL_NONCONVERTING_P (op) + && TREE_CODE (TREE_TYPE (DECL_NAME (op))) == BOOLEAN_TYPE) + { + has_bool_conversion = true; + break; + } + } + + if (!has_bool_conversion) + { + hash_map_safe_put<true> (class_type_is_boolish_cache, type, false); + return false; + } + + ops = lookup_fnfields (type, assign_op_identifier, /*protect=*/0, tf_none); + for (tree op : ovl_range (BASELINK_FUNCTIONS (ops))) + { + op = STRIP_TEMPLATE (op); + if (TREE_CODE (op) != FUNCTION_DECL) + continue; + tree parm = DECL_CHAIN (DECL_ARGUMENTS (op)); + tree parm_type = non_reference (TREE_TYPE (parm)); + if (TREE_CODE (parm_type) == BOOLEAN_TYPE) + { + has_bool_assignment = true; + break; + } + } + + bool boolish = has_bool_conversion && has_bool_assignment; + hash_map_safe_put<true> (class_type_is_boolish_cache, type, boolish); + return boolish; +} + + /* Maybe warn about an unparenthesized 'a = b' (appearing in a - boolean context where 'a == b' might have been intended). */ + boolean context where 'a == b' might have been intended). + NESTED_P is true if T appears as the RHS of another assignment. */ void -maybe_warn_unparenthesized_assignment (tree t, tsubst_flags_t complain) +maybe_warn_unparenthesized_assignment (tree t, bool nested_p, + tsubst_flags_t complain) { t = STRIP_REFERENCE_REF (t); + tree lhs; if ((complain & tf_warning) && warn_parentheses - && is_assignment_op_expr_p (t) + && is_assignment_op_expr_p (t, &lhs) /* A parenthesized expression would've had this warning suppressed by finish_parenthesized_expr. */ && !warning_suppressed_p (t, OPT_Wparentheses)) { + /* In a = b = c, don't warn if b is a boolean-like class type. */ + if (nested_p) + { + /* The caller already checked this. */ + gcc_checking_assert (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE); + + if (TREE_CODE (lhs) == ADDR_EXPR) + lhs = TREE_OPERAND (lhs, 0); + + tree lhs_type = non_reference (TREE_TYPE (lhs)); + if (CLASS_TYPE_P (lhs_type) + && class_type_is_boolish (lhs_type)) + return; + } + warning_at (cp_expr_loc_or_input_loc (t), OPT_Wparentheses, "suggest parentheses around assignment used as truth value"); suppress_warning (t, OPT_Wparentheses); @@ -903,7 +988,8 @@ maybe_convert_cond (tree cond) if (warn_sequence_point && !processing_template_decl) verify_sequence_points (cond); - maybe_warn_unparenthesized_assignment (cond, tf_warning_or_error); + maybe_warn_unparenthesized_assignment (cond, /*nested_p=*/false, + tf_warning_or_error); /* Do the conversion. */ cond = convert_from_reference (cond); diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index a6e2f4ee7da..195114e716e 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -10381,7 +10381,7 @@ convert_for_assignment (tree type, tree rhs, does not. */ if (TREE_CODE (type) == BOOLEAN_TYPE && TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE) - maybe_warn_unparenthesized_assignment (rhs, complain); + maybe_warn_unparenthesized_assignment (rhs, /*nested_p=*/true, complain); if (complain & tf_warning) warn_for_address_of_packed_member (type, rhs); diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-34.C b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C new file mode 100644 index 00000000000..2100c8a193d --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-34.C @@ -0,0 +1,31 @@ +// Verify our -Wparentheses warning handles "boolish" class types +// such as std::vector<bool>'s reference type the same as ordinary +// bool. +// { dg-additional-options "-Wparentheses" } + +#include <vector> + +void f(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +} + +template<class> +void ft(std::vector<bool> v, int i) { + bool b; + b = v[i] = true; + b = v[i] = v[i+1]; + + if (v[i] = 42) { } // { dg-message "parentheses" } + if (v[i] = v[i+1]) { } // { dg-message "parentheses" } + + if ((v[i] = 42)) { } + if ((v[i] = v[i+1])) { } +}
Bootstrap and regtesting in progress on x86_64-pc-linux-gnu, does this look OK for trunk if successful? -- >8 -- Since r14-4977-g0f2e2080685e75 the -Wparentheses warning now undesirably warns on the idiom Wparentheses-34.C:9:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses] 9 | b = v[i] = true; | ^~~~ where v has type std::vector<bool>. That commit intended to only extend the existing diagnostics so that they happen in a template context as well, but the refactoring of is_assignment_op_expr_p caused us for this particular -Wparentheses warning (from convert_for_assignment) to now consider user-defined operator= instead of just built-in operator=. And since std::vector<bool> is really a bitset, whose operator[] returns a class type with such a user-defined operator= (taking bool), we now warn. But arguably "boolish" class types should be treated like ordinary bool as far as the warning is concerned. To that end this patch relaxes the warning for such types, specifically when the (class) type can be (implicitly) converted to a and assigned from a bool. This should cover at least implementations of std::vector<bool>::reference. gcc/cp/ChangeLog: * cp-tree.h (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. * semantics.cc (is_assignment_op_expr_p): Add 'rhs' bool parameter and set it accordingly. (class_type_is_boolish_cache): Define. (class_type_is_boolish): Define. (maybe_warn_unparenthesized_assignment): Add 'nested_p' bool parameter. Relax the warning for nested assignments to boolean-like class types. (maybe_convert_cond): Pass nested_p=false to maybe_warn_unparenthesized_assignment. * typeck.cc (convert_for_assignment): Pass nested_p=true to maybe_warn_unparenthesized_assignment. gcc/testsuite/ChangeLog: * g++.dg/warn/Wparentheses-34.C: New test. --- gcc/cp/cp-tree.h | 2 +- gcc/cp/semantics.cc | 106 ++++++++++++++++++-- gcc/cp/typeck.cc | 2 +- gcc/testsuite/g++.dg/warn/Wparentheses-34.C | 31 ++++++ 4 files changed, 129 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-34.C