Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

[-- Attachment #1: Type: text/plain, Size: 3160 bytes --]

Before I start, sincere apologies for the email mishaps! I was setting up
an email client and somehow the emails I sent did not initially seem to
go through, but they actually did. You might have received several
duplicate emails as a result.

On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> > Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> > subject with "c:",
> > but I just went with it as I didn't know any better. Unfortunately, I
> > can't change it now on the current thread.
>
> That came from this line in the testcase:
>
>  > +/* PR c/25689 */
>
> The PR should be c++/25689.  Also, sometimes the bugzilla component
> isn't the same as the area of the compiler you're changing; the latter
> is what you want in the patch subject, so that the right people know to
> review it.

Oh, I see. Thanks for the explanation. I've fixed the line.

> > Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> > mailing list setup so there are some kinks I have to iron out.
>
> FWIW it's often easier to send the patch as an attachment.

Alright, I'll send patches as attachments instead. I originally sent
them as text as it is easier to comment on them.
> > +/* Test non-empty class */
> > +void f2(B b1, B b2)
> > +{
> > + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
> > + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
> > + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
> > + if (b1.operator= (0));
> > +
> > + /* Ideally, we wouldn't warn for non-empty classes using trivial
> > +  operator= (below), but we currently do as it is a MODIFY_EXPR. */
> > + // if (b1.operator= (b2));
>
> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> build_over_call.

Unfortunately, that also affects the warning for if (b1 = b2) just 5
lines above. Both expressions seem to generate the same tree structure.

v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
1. Check for error_mark_node in is_assignment_op_expr_pr.
2. Change "c:" to "c++:".

v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html
Changes since v5:
1. Revert changes in v4.
2. Replace gcc_assert with a return NULL_TREE in extract_call_expr.

v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html
Changes since v4:
1. Refactor the non-assert-related code out of extract_call_expr and
   call that function instead to check for call expressions.

v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html
Changes since v3:
1. Also handle COMPOUND_EXPRs and TARGET_EXPRs.

v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html
Changes since v2:
1. Add more test cases in Wparentheses-31.C.
2. Refactor added logic to a function (is_assignment_overload_ref_p).
3. Use REFERENCE_REF_P instead of INDIRECT_REF_P.

v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html
Changes since v1:
1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit
   operator=() calls.
2. Use INDIRECT_REF_P to filter implicit operator=() calls.
3. Use cp_get_callee_fndecl_nofold.
4. Add spaces before (.

[-- Attachment #2: 0001-c-Add-diagnostic-when-operator-is-used-as-truth-cond.patch --]
[-- Type: application/x-patch, Size: 5231 bytes --]

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/16/22 02:16, Zhao Wei Liew wrote:
> On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
>>> Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
>>> subject with "c:",
>>> but I just went with it as I didn't know any better. Unfortunately, I
>>> can't change it now on the current thread.
>>
>> That came from this line in the testcase:
>>
>>   > +/* PR c/25689 */
>>
>> The PR should be c++/25689.  Also, sometimes the bugzilla component
>> isn't the same as the area of the compiler you're changing; the latter
>> is what you want in the patch subject, so that the right people know to
>> review it.
> 
> Oh, I see. Thanks for the explanation. I've fixed the line.
> 
>>> Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
>>> mailing list setup so there are some kinks I have to iron out.
>>
>> FWIW it's often easier to send the patch as an attachment.
> 
> Alright, I'll send patches as attachments instead. I originally sent
> them as text as it is easier to comment on them.

It is a bit more of a hassle in this case because your mail sender 
doesn't mark the patch as text, but rather application/mbox or 
application/x-patch, so my mail reader for patch review (Thunderbird) 
doesn't display it inline.  I tried sending myself a patch through the 
gmail web interface, and it used text/x-patch, which is OK; what are you 
using to send?

Maybe renaming the file to .txt before sending would help?

>>> +/* Test non-empty class */
>>> +void f2(B b1, B b2)
>>> +{
>>> + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
>>> + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
>>> + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
>>> + if (b1.operator= (0));
>>> +
>>> + /* Ideally, we wouldn't warn for non-empty classes using trivial
>>> +  operator= (below), but we currently do as it is a MODIFY_EXPR. */
>>> + // if (b1.operator= (b2));
>>
>> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
>> build_over_call.
> 
> Unfortunately, that also affects the warning for if (b1 = b2) just 5
> lines above. Both expressions seem to generate the same tree structure.

True, you would need to put the call to suppress_warning in build_new_op 
around where CALL_EXPR_OPERATOR_SYNTAX is set.

Jason

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

On Thu, 17 Feb 2022 at 00:59, Jason Merrill <jason@redhat.com> wrote:
>
> On 2/16/22 02:16, Zhao Wei Liew wrote:
> > On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> >>> Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> >>> subject with "c:",
> >>> but I just went with it as I didn't know any better. Unfortunately, I
> >>> can't change it now on the current thread.
> >>
> >> That came from this line in the testcase:
> >>
> >>   > +/* PR c/25689 */
> >>
> >> The PR should be c++/25689.  Also, sometimes the bugzilla component
> >> isn't the same as the area of the compiler you're changing; the latter
> >> is what you want in the patch subject, so that the right people know to
> >> review it.
> >
> > Oh, I see. Thanks for the explanation. I've fixed the line.
> >
> >>> Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> >>> mailing list setup so there are some kinks I have to iron out.
> >>
> >> FWIW it's often easier to send the patch as an attachment.
> >
> > Alright, I'll send patches as attachments instead. I originally sent
> > them as text as it is easier to comment on them.
>
> It is a bit more of a hassle in this case because your mail sender
> doesn't mark the patch as text, but rather application/mbox or
> application/x-patch, so my mail reader for patch review (Thunderbird)
> doesn't display it inline.  I tried sending myself a patch through the
> gmail web interface, and it used text/x-patch, which is OK; what are you
> using to send?
>
> Maybe renaming the file to .txt before sending would help?

Hmm, in the end I used gmail to send the patch, so I'm not sure why it was
marked that way. I'll test it out again before sending another patch.

> >>> +/* Test non-empty class */
> >>> +void f2(B b1, B b2)
> >>> +{
> >>> + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
> >>> + if (b1.operator= (0));
> >>> +
> >>> + /* Ideally, we wouldn't warn for non-empty classes using trivial
> >>> +  operator= (below), but we currently do as it is a MODIFY_EXPR. */
> >>> + // if (b1.operator= (b2));
> >>
> >> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> >> build_over_call.
> >
> > Unfortunately, that also affects the warning for if (b1 = b2) just 5
> > lines above. Both expressions seem to generate the same tree structure.
>
> True, you would need to put the call to suppress_warning in build_new_op
> around where CALL_EXPR_OPERATOR_SYNTAX is set.

It seems like that would suppress the warning for the case of if (b1 = b2)
instead of
if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
in build_method_call instead?

This is what I've tried so far:

1. Call suppress_warning (result, ...) in the trivial_fn_p block in
build_new_op,
   right above the comment "There won't be a CALL_EXPR" (line 6699).
   This suppresses the warning for if (b1 = b2) but not for if
(b1.operator= (b2)).

2. Call suppress_warning (result, ...) in build_method_call, right after
the call to
    build_over_call (line 11141). This suppresses the warning for if
(b1.operator= (b2))
    and not if (b1 = b2).

Based on this, I think the 2nd option might be what we want here? Please
correct me if I'm
wrong. I'm also unsure if there are issues that might arise with this
change.

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]

On Fri, 18 Feb 2022 at 08:32, Zhao Wei Liew <zhaoweiliew@gmail.com> wrote:
>
> > >>> +/* Test non-empty class */
> > >>> +void f2(B b1, B b2)
> > >>> +{
> > >>> + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
> > >>> + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
> > >>> + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
> > >>> + if (b1.operator= (0));
> > >>> +
> > >>> + /* Ideally, we wouldn't warn for non-empty classes using trivial
> > >>> +  operator= (below), but we currently do as it is a MODIFY_EXPR. */
> > >>> + // if (b1.operator= (b2));
> > >>
> > >> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> > >> build_over_call.
> > >
> > > Unfortunately, that also affects the warning for if (b1 = b2) just 5
> > > lines above. Both expressions seem to generate the same tree structure.
> >
> > True, you would need to put the call to suppress_warning in build_new_op
> > around where CALL_EXPR_OPERATOR_SYNTAX is set.
>
> It seems like that would suppress the warning for the case of if (b1 = b2) instead of
> if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
> in build_method_call instead?
>
> This is what I've tried so far:
>
> 1. Call suppress_warning (result, ...) in the trivial_fn_p block in build_new_op,
>    right above the comment "There won't be a CALL_EXPR" (line 6699).
>    This suppresses the warning for if (b1 = b2) but not for if (b1.operator= (b2)).
>
> 2. Call suppress_warning (result, ...) in build_method_call, right after the call to
>     build_over_call (line 11141). This suppresses the warning for if (b1.operator= (b2))
>     and not if (b1 = b2).
>
> Based on this, I think the 2nd option might be what we want here? Please correct me if I'm
> wrong. I'm also unsure if there are issues that might arise with this change.

To better illustrate the 2nd option, I've attached it as a patch v8.
How does it look?

v7: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590464.html
Changes since v7:
1. Suppress -Wparentheses warnings in build_new_method_call.
2. Uncomment the test case for if (b1.operator= (b2)).

v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
Changes since v6:
1. Check for error_mark_node in is_assignment_op_expr_pr.
2. Change "c:" to "c++:".

v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html
Changes since v5:
1. Revert changes in v4.
2. Replace gcc_assert with a return NULL_TREE in extract_call_expr.

v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html
Changes since v4:
1. Refactor the non-assert-related code out of extract_call_expr and
   call that function instead to check for call expressions.

v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html
Changes since v3:
1. Also handle COMPOUND_EXPRs and TARGET_EXPRs.

v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html
Changes since v2:
1. Add more test cases in Wparentheses-31.C.
2. Refactor added logic to a function (is_assignment_overload_ref_p).
3. Use REFERENCE_REF_P instead of INDIRECT_REF_P.

v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html
Changes since v1:
1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit
   operator=() calls.
2. Use INDIRECT_REF_P to filter implicit operator=() calls.
3. Use cp_get_callee_fndecl_nofold.
4. Add spaces before (.

[-- Attachment #2: 0001-c-Add-diagnostic-when-operator-is-used-as-truth-cond.patch --]
[-- Type: text/x-patch, Size: 5608 bytes --]

From ef4cfecca64b2cb199a5d3979fe99f8c9bd0f414 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew <zhaoweiliew@gmail.com>
Date: Tue, 15 Feb 2022 17:44:29 +0800
Subject: [PATCH] c++: Add diagnostic when operator= is used as truth cond
 [PR25689]

When compiling the following code with g++ -Wparentheses, GCC does not
warn on the if statement. For example, there is no warning for this code:

struct A {
	A& operator=(int);
	operator bool();
};

void f(A a) {
	if (a = 0); // no warning
}

This is because a = 0 is a call to operator=, which GCC does not handle.

This patch fixes this issue by handling calls to operator= when deciding
to warn.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

	PR c++/25689

gcc/cp/ChangeLog:

	* call.cc (extract_call_expr): Return a NULL_TREE on failure
	  instead of asserting.
	* semantics.cc (is_assignment_op_expr_p): Add function to check
	  if an expression is a call to an op= operator expression.
	(maybe_convert_cond): Handle the case of a op= operator expression
	  for the -Wparentheses diagnostic.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.

Signed-off-by: Zhao Wei Liew <zhaoweiliew@gmail.com>
---
 gcc/cp/call.cc                              | 12 +++--
 gcc/cp/semantics.cc                         | 22 +++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 59 +++++++++++++++++++++
 3 files changed, 89 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index d6eed5ed835..caf22e02b39 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7090,9 +7090,10 @@ extract_call_expr (tree call)
       default:;
       }
 
-  gcc_assert (TREE_CODE (call) == CALL_EXPR
-	      || TREE_CODE (call) == AGGR_INIT_EXPR
-	      || call == error_mark_node);
+  if (TREE_CODE (call) != CALL_EXPR
+      && TREE_CODE (call) != AGGR_INIT_EXPR
+      && call != error_mark_node)
+    return NULL_TREE;
   return call;
 }
 
@@ -11137,6 +11138,11 @@ build_new_method_call (tree instance, tree fns, vec<tree, va_gc> **args,
 		*fn_p = fn;
 	      /* Build the actual CALL_EXPR.  */
 	      call = build_over_call (cand, flags, complain);
+
+	      /* Suppress warnings for if (my_struct.operator= (x)) where
+					 my_struct is implicitly converted to bool. */
+	      suppress_warning (call, OPT_Wparentheses);
+
 	      /* In an expression of the form `a->f()' where `f' turns
 		 out to be a static member function, `a' is
 		 none-the-less evaluated.  */
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab..9cd88715417 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,26 @@ 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. */
+static bool
+is_assignment_op_expr_p (tree call)
+{
+  if (call == NULL_TREE)
+    return false;
+
+  call = extract_call_expr (call);
+  if (call == NULL_TREE
+      || call == error_mark_node
+      || !CALL_EXPR_OPERATOR_SYNTAX (call))
+    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);
+}
+
 /* COND is the condition-expression for an if, while, etc.,
    statement.  Convert it to a boolean value, if appropriate.
    In addition, verify sequence points if -Wsequence-point is enabled.  */
@@ -836,7 +856,7 @@ maybe_convert_cond (tree cond)
   /* Do the conversion.  */
   cond = convert_from_reference (cond);
 
-  if (TREE_CODE (cond) == MODIFY_EXPR
+  if ((TREE_CODE (cond) == MODIFY_EXPR || is_assignment_op_expr_p (cond))
       && warn_parentheses
       && !warning_suppressed_p (cond, OPT_Wparentheses)
       && warning_at (cp_expr_loc_or_input_loc (cond),
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
new file mode 100644
index 00000000000..6b5ce3c0e6b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,59 @@
+/* Test that -Wparentheses warns for struct/class assignments,
+   except for explicit calls to operator= (). */
+/* PR c++/25689 */
+/* { dg-options "-Wparentheses" }  */
+
+struct A
+{
+	A& operator= (int);
+	A operator= (double);
+	operator bool ();
+};
+
+struct B
+{
+	bool x;
+	B& operator= (int);
+	B operator= (double);
+	operator bool ();
+};
+
+struct C
+{
+	C& operator= (int);
+	virtual C operator= (double);
+	operator bool ();
+};
+
+/* Test empty class */
+void f1 (A a1, A a2)
+{
+	if (a1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (a1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (a1.operator= (0));
+	if (a1.operator= (a2));
+
+	/* Ideally, we'd warn for empty classes using trivial operator= (below),
+	   but we don't do so yet as it is a non-trivial COMPOUND_EXPR. */
+	// if (a1 = a2); 
+}
+
+/* Test non-empty class */
+void f2 (B b1, B b2)
+{
+	if (b1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (b1 = b2); /* { dg-warning "suggest parentheses" } */
+	if (b1.operator= (0));
+	if (b1.operator= (b2));
+}
+
+/* Test class with vtable */
+void f3 (C c1, C c2)
+{
+	if (c1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (c1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (c1 = c2); /* { dg-warning "suggest parentheses" } */
+	if (c1.operator= (0));
+	if (c1.operator= (c2));
+}
-- 
2.35.1

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/17/22 23:30, Zhao Wei Liew wrote:
> On Fri, 18 Feb 2022 at 08:32, Zhao Wei Liew <zhaoweiliew@gmail.com> wrote:
>>
>>>>>> +/* Test non-empty class */
>>>>>> +void f2(B b1, B b2)
>>>>>> +{
>>>>>> + if (b1 = 0); /* { dg-warning "suggest parentheses" } */
>>>>>> + if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
>>>>>> + if (b1 = b2); /* { dg-warning "suggest parentheses" } */
>>>>>> + if (b1.operator= (0));
>>>>>> +
>>>>>> + /* Ideally, we wouldn't warn for non-empty classes using trivial
>>>>>> +  operator= (below), but we currently do as it is a MODIFY_EXPR. */
>>>>>> + // if (b1.operator= (b2));
>>>>>
>>>>> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
>>>>> build_over_call.
>>>>
>>>> Unfortunately, that also affects the warning for if (b1 = b2) just 5
>>>> lines above. Both expressions seem to generate the same tree structure.
>>>
>>> True, you would need to put the call to suppress_warning in build_new_op
>>> around where CALL_EXPR_OPERATOR_SYNTAX is set.
>>
>> It seems like that would suppress the warning for the case of if (b1 = b2) instead of
>> if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
>> in build_method_call instead?
>>
>> This is what I've tried so far:
>>
>> 1. Call suppress_warning (result, ...) in the trivial_fn_p block in build_new_op,
>>     right above the comment "There won't be a CALL_EXPR" (line 6699).
>>     This suppresses the warning for if (b1 = b2) but not for if (b1.operator= (b2)).
>>
>> 2. Call suppress_warning (result, ...) in build_method_call, right after the call to
>>      build_over_call (line 11141). This suppresses the warning for if (b1.operator= (b2))
>>      and not if (b1 = b2).
>>
>> Based on this, I think the 2nd option might be what we want here? Please correct me if I'm
>> wrong. I'm also unsure if there are issues that might arise with this change.
> 
> To better illustrate the 2nd option, I've attached it as a patch v8.
> How does it look?

It looks good, but unfortunately regresses some other warning tests, 
such as Wnonnull5.C.  Please remember to run the regression tests before 
sending a patch (https://gcc.gnu.org/contribute.html#testing).

This seems to be a complicated problem with suppress_warning, which 
means your call to suppress_warning effectively silences all later 
warnings, not just -Wparentheses.

You should be able to work around this issue by only calling 
suppress_warning in the specific case we're interested in, i.e. when 
warn_parentheses is enabled and "call" is a MODIFY_EXPR.

> v7: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590464.html
> Changes since v7:
> 1. Suppress -Wparentheses warnings in build_new_method_call.
> 2. Uncomment the test case for if (b1.operator= (b2)).
> 
> v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
> Changes since v6:
> 1. Check for error_mark_node in is_assignment_op_expr_pr.
> 2. Change "c:" to "c++:".
> 
> v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html
> Changes since v5:
> 1. Revert changes in v4.
> 2. Replace gcc_assert with a return NULL_TREE in extract_call_expr.
> 
> v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html
> Changes since v4:
> 1. Refactor the non-assert-related code out of extract_call_expr and
>     call that function instead to check for call expressions.
> 
> v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html
> Changes since v3:
> 1. Also handle COMPOUND_EXPRs and TARGET_EXPRs.
> 
> v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html
> Changes since v2:
> 1. Add more test cases in Wparentheses-31.C.
> 2. Refactor added logic to a function (is_assignment_overload_ref_p).
> 3. Use REFERENCE_REF_P instead of INDIRECT_REF_P.
> 
> v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html
> Changes since v1:
> 1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit
>     operator=() calls.
> 2. Use INDIRECT_REF_P to filter implicit operator=() calls.
> 3. Use cp_get_callee_fndecl_nofold.
> 4. Add spaces before (.

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

On Sat, 12 Mar 2022 at 06:15, Jason Merrill <jason@redhat.com> wrote:
> It looks good, but unfortunately regresses some other warning tests,
> such as Wnonnull5.C.  Please remember to run the regression tests before
> sending a patch (https://gcc.gnu.org/contribute.html#testing).
>
> This seems to be a complicated problem with suppress_warning, which
> means your call to suppress_warning effectively silences all later
> warnings, not just -Wparentheses.
>
> You should be able to work around this issue by only calling
> suppress_warning in the specific case we're interested in, i.e. when
> warn_parentheses is enabled and "call" is a MODIFY_EXPR.

My apologies. I've fixed the issue as you suggested and run the regression tests
to ensure no test regressions. The new patch (v9) is attached.

v8: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590570.html
Changes since v8:
1. Fix a test regression by calling suppress_warning only when "call"
is a MODIFY_EXPR.

v7: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590464.html
Changes since v7:
1. Suppress -Wparentheses warnings in build_new_method_call.
2. Uncomment the test case for if (b1.operator= (b2)).

v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
Changes since v6:
1. Check for error_mark_node in is_assignment_op_expr_pr.
2. Change "c:" to "c++:".

v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html
Changes since v5:
1. Revert changes in v4.
2. Replace gcc_assert with a return NULL_TREE in extract_call_expr.

v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html
Changes since v4:
1. Refactor the non-assert-related code out of extract_call_expr and
   call that function instead to check for call expressions.

v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html
Changes since v3:
1. Also handle COMPOUND_EXPRs and TARGET_EXPRs.

v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html
Changes since v2:
1. Add more test cases in Wparentheses-31.C.
2. Refactor added logic to a function (is_assignment_overload_ref_p).
3. Use REFERENCE_REF_P instead of INDIRECT_REF_P.

v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html
Changes since v1:
1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit
   operator=() calls.
2. Use INDIRECT_REF_P to filter implicit operator=() calls.
3. Use cp_get_callee_fndecl_nofold.
4. Add spaces before (.

[-- Attachment #2: 0001-c-Add-diagnostic-when-operator-is-used-as-truth-cond.patch.txt --]
[-- Type: text/plain, Size: 5745 bytes --]

From 28f884d51a56889e84acba970a5aac9da8b24d99 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew <zhaoweiliew@gmail.com>
Date: Tue, 15 Feb 2022 17:44:29 +0800
Subject: [PATCH] c++: Add diagnostic when operator= is used as truth cond
 [PR25689]

When compiling the following code with g++ -Wparentheses, GCC does not
warn on the if statement. For example, there is no warning for this code:

struct A {
	A& operator=(int);
	operator bool();
};

void f(A a) {
	if (a = 0); // no warning
}

This is because a = 0 is a call to operator=, which GCC does not handle.

This patch fixes this issue by handling calls to operator= when deciding
to warn.

Bootstrapped and regression tested on x86_64-pc-linux-gnu.

	PR c++/25689

gcc/cp/ChangeLog:

	* call.cc (extract_call_expr): Return a NULL_TREE on failure
	  instead of asserting.
	(build_new_method_call): Suppress -Wparentheses diagnostic for
	  MODIFY_EXPR.
	* semantics.cc (is_assignment_op_expr_p): Add function to check
	  if an expression is a call to an op= operator expression.
	(maybe_convert_cond): Handle the case of a op= operator expression
	  for the -Wparentheses diagnostic.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.

Signed-off-by: Zhao Wei Liew <zhaoweiliew@gmail.com>
---
 gcc/cp/call.cc                              | 13 +++--
 gcc/cp/semantics.cc                         | 22 +++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 59 +++++++++++++++++++++
 3 files changed, 90 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 8fe8ef306ea..f502251c291 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7101,9 +7101,10 @@ extract_call_expr (tree call)
       default:;
       }
 
-  gcc_assert (TREE_CODE (call) == CALL_EXPR
-	      || TREE_CODE (call) == AGGR_INIT_EXPR
-	      || call == error_mark_node);
+  if (TREE_CODE (call) != CALL_EXPR
+      && TREE_CODE (call) != AGGR_INIT_EXPR
+      && call != error_mark_node)
+    return NULL_TREE;
   return call;
 }
 
@@ -11148,6 +11149,12 @@ build_new_method_call (tree instance, tree fns, vec<tree, va_gc> **args,
 		*fn_p = fn;
 	      /* Build the actual CALL_EXPR.  */
 	      call = build_over_call (cand, flags, complain);
+
+	      /* Suppress warnings for if (my_struct.operator= (x)) where
+	         my_struct is implicitly converted to bool. */
+	      if (TREE_CODE (call) == MODIFY_EXPR)
+	        suppress_warning (call, OPT_Wparentheses);
+
 	      /* In an expression of the form `a->f()' where `f' turns
 		 out to be a static member function, `a' is
 		 none-the-less evaluated.  */
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index d5565ebe02d..06e1db6e49a 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,26 @@ 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. */
+static bool
+is_assignment_op_expr_p (tree call)
+{
+  if (call == NULL_TREE)
+    return false;
+
+  call = extract_call_expr (call);
+  if (call == NULL_TREE
+      || call == error_mark_node
+      || !CALL_EXPR_OPERATOR_SYNTAX (call))
+    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);
+}
+
 /* COND is the condition-expression for an if, while, etc.,
    statement.  Convert it to a boolean value, if appropriate.
    In addition, verify sequence points if -Wsequence-point is enabled.  */
@@ -836,7 +856,7 @@ maybe_convert_cond (tree cond)
   /* Do the conversion.  */
   cond = convert_from_reference (cond);
 
-  if (TREE_CODE (cond) == MODIFY_EXPR
+  if ((TREE_CODE (cond) == MODIFY_EXPR || is_assignment_op_expr_p (cond))
       && warn_parentheses
       && !warning_suppressed_p (cond, OPT_Wparentheses)
       && warning_at (cp_expr_loc_or_input_loc (cond),
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
new file mode 100644
index 00000000000..6b5ce3c0e6b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,59 @@
+/* Test that -Wparentheses warns for struct/class assignments,
+   except for explicit calls to operator= (). */
+/* PR c++/25689 */
+/* { dg-options "-Wparentheses" }  */
+
+struct A
+{
+	A& operator= (int);
+	A operator= (double);
+	operator bool ();
+};
+
+struct B
+{
+	bool x;
+	B& operator= (int);
+	B operator= (double);
+	operator bool ();
+};
+
+struct C
+{
+	C& operator= (int);
+	virtual C operator= (double);
+	operator bool ();
+};
+
+/* Test empty class */
+void f1 (A a1, A a2)
+{
+	if (a1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (a1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (a1.operator= (0));
+	if (a1.operator= (a2));
+
+	/* Ideally, we'd warn for empty classes using trivial operator= (below),
+	   but we don't do so yet as it is a non-trivial COMPOUND_EXPR. */
+	// if (a1 = a2); 
+}
+
+/* Test non-empty class */
+void f2 (B b1, B b2)
+{
+	if (b1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (b1 = b2); /* { dg-warning "suggest parentheses" } */
+	if (b1.operator= (0));
+	if (b1.operator= (b2));
+}
+
+/* Test class with vtable */
+void f3 (C c1, C c2)
+{
+	if (c1 = 0); /* { dg-warning "suggest parentheses" } */
+	if (c1 = 0.); /* { dg-warning "suggest parentheses" } */
+	if (c1 = c2); /* { dg-warning "suggest parentheses" } */
+	if (c1.operator= (0));
+	if (c1.operator= (c2));
+}
-- 
2.25.1

Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 3/13/22 19:43, Zhao Wei Liew wrote:
> On Sat, 12 Mar 2022 at 06:15, Jason Merrill <jason@redhat.com> wrote:
>> It looks good, but unfortunately regresses some other warning tests,
>> such as Wnonnull5.C.  Please remember to run the regression tests before
>> sending a patch (https://gcc.gnu.org/contribute.html#testing).
>>
>> This seems to be a complicated problem with suppress_warning, which
>> means your call to suppress_warning effectively silences all later
>> warnings, not just -Wparentheses.
>>
>> You should be able to work around this issue by only calling
>> suppress_warning in the specific case we're interested in, i.e. when
>> warn_parentheses is enabled and "call" is a MODIFY_EXPR.
> 
> My apologies. I've fixed the issue as you suggested and run the regression tests
> to ensure no test regressions. The new patch (v9) is attached.

Looks good.  One thing:

> +	/* Ideally, we'd warn for empty classes using trivial operator= (below),
> +	   but we don't do so yet as it is a non-trivial COMPOUND_EXPR. */
> +	// if (a1 = a2); 

It would be better to uncomment this line and give it an xfailed 
warning, i.e.

/* { dg-warning "suggest parentheses" "" { xfail *-*-* } } */

I've made this change and added the patch to my queue to commit when GCC 
13 stage 1 opens, probably next month.

Thanks!

Jason