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])) { }
+}
--
2.43.0.174.g055bb6e996
