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

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

[Zhao Wei Liew]

Hi!

I wrote a patch for PR 25689, but I feel like it may not be the ideal
fix. Furthermore, there are some standing issues with the patch for
which I would like tips on how to fix them.
Specifically, there are 2 issues:
1. GCC warns about  if (a.operator=(0)). That said, this may not be a
major issue as I don't think such code is widely written.
2. GCC does not warn for `if (a = b)` where the default copy/move
assignment operator is used.

I've included a code snippet in PR25689 that shows the 2 issues I
mentioned. I appreciate any feedback, thanks!

----Everything below is the actual patch----

When compiling the following code with g++ -Wparentheses, GCC does not
warn on the if statement:

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 check
for.

This patch fixes that by checking for calls to operator= when deciding
to warn.

	PR c/25689

gcc/cp/ChangeLog:

	* semantics.cc (maybe_convert_cond): Handle the operator=() case
	  as well.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.
---
 gcc/cp/semantics.cc                         | 14 +++++++++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 466d6b56871f4..6a25d039585f2 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -836,7 +836,19 @@ maybe_convert_cond (tree cond)
   /* Do the conversion.  */
   cond = convert_from_reference (cond);

-  if (TREE_CODE (cond) == MODIFY_EXPR
+  /* Also check if this is a call to operator=().
+     Example: if (my_struct = 5) {...}
+  */
+  tree fndecl = NULL_TREE;
+  if (TREE_OPERAND_LENGTH(cond) >= 1) {
+    fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0));
+  }
+
+  if ((TREE_CODE (cond) == MODIFY_EXPR
+        || (fndecl != NULL_TREE
+        && DECL_OVERLOADED_OPERATOR_P(fndecl)
+        && DECL_OVERLOADED_OPERATOR_IS(fndecl, NOP_EXPR)
+        && DECL_ASSIGNMENT_OPERATOR_P(fndecl)))
       && 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 0000000000000..abd7476ccb461
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,11 @@
+/* PR c/25689 */
+/* { dg-options "-Wparentheses" }  */
+
+struct A {
+	A& operator=(int);
+	operator bool();
+};
+
+void f(A a) {
+	if (a = 0); /* { dg-warning "suggest parentheses" } */
+}
--
2.17.1

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

[Jason Merrill]

On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote:
> Hi!
> 
> I wrote a patch for PR 25689, but I feel like it may not be the ideal
> fix. Furthermore, there are some standing issues with the patch for
> which I would like tips on how to fix them.
> Specifically, there are 2 issues:
> 1. GCC warns about  if (a.operator=(0)). That said, this may not be a
> major issue as I don't think such code is widely written.

Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX?

> 2. GCC does not warn for `if (a = b)` where the default copy/move
> assignment operator is used.

The code for trivial copy-assignment should be pretty recognizable, as a 
MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the 
comment "We must only copy the non-tail padding parts."

> I've included a code snippet in PR25689 that shows the 2 issues I
> mentioned. I appreciate any feedback, thanks!
> 
> ----Everything below is the actual patch----
> 
> When compiling the following code with g++ -Wparentheses, GCC does not
> warn on the if statement:
> 
> 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 check
> for.
> 
> This patch fixes that by checking for calls to operator= when deciding
> to warn.
> 
> 	PR c/25689
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (maybe_convert_cond): Handle the operator=() case
> 	  as well.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wparentheses-31.C: New test.
> ---
>   gcc/cp/semantics.cc                         | 14 +++++++++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++
>   2 files changed, 24 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 466d6b56871f4..6a25d039585f2 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -836,7 +836,19 @@ maybe_convert_cond (tree cond)
>     /* Do the conversion.  */
>     cond = convert_from_reference (cond);
> 
> -  if (TREE_CODE (cond) == MODIFY_EXPR
> +  /* Also check if this is a call to operator=().
> +     Example: if (my_struct = 5) {...}
> +  */
> +  tree fndecl = NULL_TREE;
> +  if (TREE_OPERAND_LENGTH(cond) >= 1) {
> +    fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0));

Let's use cp_get_callee_fndecl_nofold.

Please add a space before all (

> +  }
> +
> +  if ((TREE_CODE (cond) == MODIFY_EXPR
> +        || (fndecl != NULL_TREE
> +        && DECL_OVERLOADED_OPERATOR_P(fndecl)
> +        && DECL_OVERLOADED_OPERATOR_IS(fndecl, NOP_EXPR)
> +        && DECL_ASSIGNMENT_OPERATOR_P(fndecl)))
>         && 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 0000000000000..abd7476ccb461
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,11 @@
> +/* PR c/25689 */
> +/* { dg-options "-Wparentheses" }  */
> +
> +struct A {
> +	A& operator=(int);
> +	operator bool();
> +};
> +
> +void f(A a) {
> +	if (a = 0); /* { dg-warning "suggest parentheses" } */
> +}
> --
> 2.17.1
> 

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

[Zhao Wei Liew]

On Fri, 11 Feb 2022 at 00:14, Jason Merrill <jason@redhat.com> wrote:
>
> On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote:
> > Hi!
> >
> > I wrote a patch for PR 25689, but I feel like it may not be the ideal
> > fix. Furthermore, there are some standing issues with the patch for
> > which I would like tips on how to fix them.
> > Specifically, there are 2 issues:
> > 1. GCC warns about  if (a.operator=(0)). That said, this may not be a
> > major issue as I don't think such code is widely written.
>
> Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX?

Thanks! It worked. There is no longer a warning for `if (a.operator=(0))`.
The updated patch is at the bottom of this email.

>
> > 2. GCC does not warn for `if (a = b)` where the default copy/move
> > assignment operator is used.
>
> The code for trivial copy-assignment should be pretty recognizable, as a
> MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the
> comment "We must only copy the non-tail padding parts."

Ah, I see what you mean. Thanks! However, it seems like that's the case only
for non-empty classes. GCC already warns for MODIFY_EXPR, so there's
nothing we need to do there.

On the other hand, for empty classes, it seems that a COMPOUND_EXPR
is built in build_over_call under the is_really_empty_class guard (line 9791).
I don't understand the tree structure that I should identify though.
Could you give me any further explanations on that?

> > -  if (TREE_CODE (cond) == MODIFY_EXPR
> > +  /* Also check if this is a call to operator=().
> > +     Example: if (my_struct = 5) {...}
> > +  */
> > +  tree fndecl = NULL_TREE;
> > +  if (TREE_OPERAND_LENGTH(cond) >= 1) {
> > +    fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0));
>
> Let's use cp_get_callee_fndecl_nofold.
>
> Please add a space before all (

Got it. May I know why it's better to use *_nofold here?

On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond)
instead of TREE_OPERAND_LENGTH (cond) >= 1.
I hope that's better for semantics.

------Everything below is the updated patch-----

When compiling the following code with g++ -Wparentheses, GCC does not
warn on the if statement:

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 check
for.

This patch fixes that by checking for calls to operator= when deciding
to warn.

v1: 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 (.

	PR c/25689

gcc/cp/ChangeLog:

	* semantics.cc (maybe_convert_cond): Handle the operator=() case
	  as well.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.
---
 gcc/cp/semantics.cc                         | 18 +++++++++++++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 466d6b56871f4..b45903a6a6fde 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -836,7 +836,23 @@ maybe_convert_cond (tree cond)
   /* Do the conversion.  */
   cond = convert_from_reference (cond);

-  if (TREE_CODE (cond) == MODIFY_EXPR
+  /* Check for operator syntax calls to operator=().
+     Example: if (my_struct = 5) {...}
+  */
+  tree fndecl = NULL_TREE;
+  if (INDIRECT_REF_P (cond)) {
+    tree fn = TREE_OPERAND (cond, 0);
+    if (TREE_CODE (fn) == CALL_EXPR
+        && CALL_EXPR_OPERATOR_SYNTAX (fn)) {
+      fndecl = cp_get_callee_fndecl_nofold (fn);
+    }
+  }
+
+  if ((TREE_CODE (cond) == MODIFY_EXPR
+        || (fndecl != NULL_TREE
+        && DECL_OVERLOADED_OPERATOR_P (fndecl)
+        && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
+        && DECL_ASSIGNMENT_OPERATOR_P (fndecl)))
       && 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 0000000000000..abd7476ccb461
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,11 @@
+/* PR c/25689 */
+/* { dg-options "-Wparentheses" }  */
+
+struct A {
+	A& operator=(int);
+	operator bool();
+};
+
+void f(A a) {
+	if (a = 0); /* { dg-warning "suggest parentheses" } */
+}
--
2.17.1

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

[Jason Merrill]

On 2/10/22 23:01, Zhao Wei Liew wrote:
> On Fri, 11 Feb 2022 at 00:14, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 2/9/22 21:18, Zhao Wei Liew via Gcc-patches wrote:
>>> Hi!
>>>
>>> I wrote a patch for PR 25689, but I feel like it may not be the ideal
>>> fix. Furthermore, there are some standing issues with the patch for
>>> which I would like tips on how to fix them.
>>> Specifically, there are 2 issues:
>>> 1. GCC warns about  if (a.operator=(0)). That said, this may not be a
>>> major issue as I don't think such code is widely written.
>>
>> Can you avoid this by checking CALL_EXPR_OPERATOR_SYNTAX?
> 
> Thanks! It worked. There is no longer a warning for `if (a.operator=(0))`.
> The updated patch is at the bottom of this email.
> 
>>
>>> 2. GCC does not warn for `if (a = b)` where the default copy/move
>>> assignment operator is used.
>>
>> The code for trivial copy-assignment should be pretty recognizable, as a
>> MODIFY_EXPR of two MEM_REFs; it's built in build_over_call after the
>> comment "We must only copy the non-tail padding parts."
> 
> Ah, I see what you mean. Thanks! However, it seems like that's the case only
> for non-empty classes. GCC already warns for MODIFY_EXPR, so there's
> nothing we need to do there.
> 
> On the other hand, for empty classes, it seems that a COMPOUND_EXPR
> is built in build_over_call under the is_really_empty_class guard (line 9791).
> I don't understand the tree structure that I should identify though.
> Could you give me any further explanations on that?

True, that's not very recognizable.  I suppose we could use a 
TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty 
classes alone; neither assignment nor comparison of empty classes should 
happen much in practice.

>>> -  if (TREE_CODE (cond) == MODIFY_EXPR
>>> +  /* Also check if this is a call to operator=().
>>> +     Example: if (my_struct = 5) {...}
>>> +  */
>>> +  tree fndecl = NULL_TREE;
>>> +  if (TREE_OPERAND_LENGTH(cond) >= 1) {
>>> +    fndecl = cp_get_callee_fndecl(TREE_OPERAND(cond, 0));
>>
>> Let's use cp_get_callee_fndecl_nofold.
>>
>> Please add a space before all (
> 
> Got it. May I know why it's better to use *_nofold here?

To avoid trying to do constexpr evaluation of the function operand when 
it's non-constant; in the interesting cases it won't be needed.

However, have you tested virtual operator=?

> On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond)
> instead of TREE_OPERAND_LENGTH (cond) >= 1.
> I hope that's better for semantics.

Yes; REFERENCE_REF_P might be even better.

> ------Everything below is the updated patch-----
> 
> When compiling the following code with g++ -Wparentheses, GCC does not
> warn on the if statement:
> 
> 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 check
> for.
> 
> This patch fixes that by checking for calls to operator= when deciding
> to warn.
> 
> v1: 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 (.
> 
> 	PR c/25689
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (maybe_convert_cond): Handle the operator=() case
> 	  as well.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wparentheses-31.C: New test.
> ---
>   gcc/cp/semantics.cc                         | 18 +++++++++++++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++++++++++
>   2 files changed, 28 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 466d6b56871f4..b45903a6a6fde 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -836,7 +836,23 @@ maybe_convert_cond (tree cond)
>     /* Do the conversion.  */
>     cond = convert_from_reference (cond);
> 
> -  if (TREE_CODE (cond) == MODIFY_EXPR
> +  /* Check for operator syntax calls to operator=().
> +     Example: if (my_struct = 5) {...}
> +  */
> +  tree fndecl = NULL_TREE;
> +  if (INDIRECT_REF_P (cond)) {

The { should go on the next line.

> +    tree fn = TREE_OPERAND (cond, 0);
> +    if (TREE_CODE (fn) == CALL_EXPR
> +        && CALL_EXPR_OPERATOR_SYNTAX (fn)) {
> +      fndecl = cp_get_callee_fndecl_nofold (fn);
> +    }
> +  }
> +
> +  if ((TREE_CODE (cond) == MODIFY_EXPR
> +        || (fndecl != NULL_TREE
> +        && DECL_OVERLOADED_OPERATOR_P (fndecl)
> +        && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
> +        && DECL_ASSIGNMENT_OPERATOR_P (fndecl)))
>         && 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 0000000000000..abd7476ccb461
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,11 @@
> +/* PR c/25689 */
> +/* { dg-options "-Wparentheses" }  */
> +
> +struct A {
> +	A& operator=(int);
> +	operator bool();
> +};
> +
> +void f(A a) {
> +	if (a = 0); /* { dg-warning "suggest parentheses" } */
> +}
> --
> 2.17.1
> 

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

[Zhao Wei Liew]

On Fri, 11 Feb 2022 at 20:47, Jason Merrill <jason@redhat.com> wrote:
> >
> > On the other hand, for empty classes, it seems that a COMPOUND_EXPR
> > is built in build_over_call under the is_really_empty_class guard (line 9791).
> > I don't understand the tree structure that I should identify though.
> > Could you give me any further explanations on that?
>
> True, that's not very recognizable.  I suppose we could use a
> TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty
> classes alone; neither assignment nor comparison of empty classes should
> happen much in practice.

I agree. I'll leave them alone.

> >
> > Got it. May I know why it's better to use *_nofold here?
>
> To avoid trying to do constexpr evaluation of the function operand when
> it's non-constant; in the interesting cases it won't be needed.
>
> However, have you tested virtual operator=?
>

Yup, everything seems to work as expected for structs using virtual operator=.
I've updated the test suite to reflect the tests.

One thing to note: I've commented out 2 test statements that shouldn't
work. One of them
is caused by trivial assignments in empty classes being COMPOUND_EXPRs as we
discussed above. The other is an existing issue caused by trivial
assignments in non-empty
classes being MODIFY_EXPRs.

> > On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond)
> > instead of TREE_OPERAND_LENGTH (cond) >= 1.
> > I hope that's better for semantics.
>
> Yes; REFERENCE_REF_P might be even better.
>

Alright, I've changed it to REFERENCE_REF_P and it seems to work fine as well.



-----Everything below is the actual patch v3-----



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-pc-linux-gnu.

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 (.

	PR c/25689

gcc/cp/ChangeLog:

	* semantics.cc (maybe_convert_cond): Handle the implicit
	  operator=() case for -Wparentheses.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.
---
 gcc/cp/semantics.cc                         | 21 +++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 55 +++++++++++++++++++++
 2 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab..30ffb23a032 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,25 @@ finish_goto_stmt (tree destination)
   return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
 }

+/* Returns true if EXPR is a reference to an implicit
+   call to operator=(). */
+static bool
+is_assignment_overload_ref_p (tree expr)
+{
+  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
+    return false;
+
+  tree fn = TREE_OPERAND (expr, 0);
+  if (TREE_CODE (fn) != CALL_EXPR || !CALL_EXPR_OPERATOR_SYNTAX (fn))
+    return false;
+
+  tree fndecl = cp_get_callee_fndecl_nofold (fn);
+  return fndecl != NULL_TREE
+    && DECL_OVERLOADED_OPERATOR_P (fndecl)
+    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
+    && DECL_ASSIGNMENT_OPERATOR_P (fndecl);
+}
+
 /* 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 +855,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_overload_ref_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..7a5789fb7a1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,55 @@
+/* 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);
+	operator bool ();
+};
+
+struct B
+{
+	bool x;
+	B& operator= (int);
+	operator bool ();
+};
+
+struct C
+{
+	C& operator= (int);
+	virtual C& operator= (double);
+	operator bool ();
+};
+
+void f1 (A a1, A a2)
+{
+	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);
+}
+
+void f2(B b1, B b2)
+{
+	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));
+}
+
+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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/12/22 01:59, Zhao Wei Liew wrote:
> On Fri, 11 Feb 2022 at 20:47, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> On the other hand, for empty classes, it seems that a COMPOUND_EXPR
>>> is built in build_over_call under the is_really_empty_class guard (line 9791).
>>> I don't understand the tree structure that I should identify though.
>>> Could you give me any further explanations on that?
>>
>> True, that's not very recognizable.  I suppose we could use a
>> TREE_LANG_FLAG to mark that COMPOUND_EXPR, or we could leave empty
>> classes alone; neither assignment nor comparison of empty classes should
>> happen much in practice.
> 
> I agree. I'll leave them alone.
> 
>>>
>>> Got it. May I know why it's better to use *_nofold here?
>>
>> To avoid trying to do constexpr evaluation of the function operand when
>> it's non-constant; in the interesting cases it won't be needed.
>>
>> However, have you tested virtual operator=?
>>
> 
> Yup, everything seems to work as expected for structs using virtual operator=.
> I've updated the test suite to reflect the tests.
> 
> One thing to note: I've commented out 2 test statements that shouldn't
> work. One of them
> is caused by trivial assignments in empty classes being COMPOUND_EXPRs as we
> discussed above. The other is an existing issue caused by trivial
> assignments in non-empty
> classes being MODIFY_EXPRs.
> 
>>> On an unrelated note, I adjusted the if condition to use INDIRECT_REF_P (cond)
>>> instead of TREE_OPERAND_LENGTH (cond) >= 1.
>>> I hope that's better for semantics.
>>
>> Yes; REFERENCE_REF_P might be even better.
>>
> 
> Alright, I've changed it to REFERENCE_REF_P and it seems to work fine as well.
> 
> 
> 
> -----Everything below is the actual patch v3-----
> 
> 
> 
> 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-pc-linux-gnu.
> 
> 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 (.
> 
> 	PR c/25689
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (maybe_convert_cond): Handle the implicit
> 	  operator=() case for -Wparentheses.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wparentheses-31.C: New test.
> ---
>   gcc/cp/semantics.cc                         | 21 +++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 55 +++++++++++++++++++++
>   2 files changed, 75 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab..30ffb23a032 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -815,6 +815,25 @@ finish_goto_stmt (tree destination)
>     return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
>   }
> 
> +/* Returns true if EXPR is a reference to an implicit
> +   call to operator=(). */
> +static bool
> +is_assignment_overload_ref_p (tree expr)
> +{
> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
> +    return false;

This will only warn about op= that returns a reference, which is not 
required.

> +  tree fn = TREE_OPERAND (expr, 0);
> +  if (TREE_CODE (fn) != CALL_EXPR || !CALL_EXPR_OPERATOR_SYNTAX (fn))
> +    return false;
> +
> +  tree fndecl = cp_get_callee_fndecl_nofold (fn);
> +  return fndecl != NULL_TREE
> +    && DECL_OVERLOADED_OPERATOR_P (fndecl)
> +    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
> +    && DECL_ASSIGNMENT_OPERATOR_P (fndecl);

This could be

       && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
       && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);

without the separate DECL_OVERLOADED_OPERATOR_P test.

> +}
> +
>   /* 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 +855,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_overload_ref_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..7a5789fb7a1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,55 @@
> +/* 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);
> +	operator bool ();
> +};
> +
> +struct B
> +{
> +	bool x;
> +	B& operator= (int);
> +	operator bool ();
> +};
> +
> +struct C
> +{
> +	C& operator= (int);
> +	virtual C& operator= (double);
> +	operator bool ();
> +};
> +
> +void f1 (A a1, A a2)
> +{
> +	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);
> +}
> +
> +void f2(B b1, B b2)
> +{
> +	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));
> +}
> +
> +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));
> +}

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

[Zhao Wei Liew]

On 14/02/2022, Jason Merrill <jason@redhat.com> wrote:
>>
>> +/* Returns true if EXPR is a reference to an implicit
>> +   call to operator=(). */
>> +static bool
>> +is_assignment_overload_ref_p (tree expr)
>> +{
>> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
>> +    return false;
>
> This will only warn about op= that returns a reference, which is not
> required.
>

Ah I understand now. I added some new test cases for non-reference return types
and copied some code from extract_call_expr that seems to do what we want.

>> +  return fndecl != NULL_TREE
>> +    && DECL_OVERLOADED_OPERATOR_P (fndecl)
>> +    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
>> +    && DECL_ASSIGNMENT_OPERATOR_P (fndecl);
>
> This could be
>
>        && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
>        && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
>
> without the separate DECL_OVERLOADED_OPERATOR_P test.
>

I see. I've adjusted the code as you suggested and it works. Thanks!

------ Everything below is the patch v4 ------

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-pc-linux-gnu.

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 (.

	PR c/25689

gcc/cp/ChangeLog:

	* semantics.cc (maybe_convert_cond): Handle the implicit
	  operator=() case for -Wparentheses.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wparentheses-31.C: New test.
---
 gcc/cp/semantics.cc                         | 29 +++++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
 2 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab1c..9c3f613a1aa62 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,33 @@ 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
+   to operator=() that is written as an operator expression. */
+static bool
+is_assignment_op_expr_p(tree call)
+{
+  if (call == NULL_TREE)
+    return false;
+
+  /* Unwrap the CALL_EXPR. */
+  while (TREE_CODE (call) == COMPOUND_EXPR)
+    call = TREE_OPERAND (call, 1);
+  if (REFERENCE_REF_P (call))
+    call = TREE_OPERAND (call, 0);
+  if (TREE_CODE (call) == TARGET_EXPR)
+    call = TARGET_EXPR_INITIAL (call);
+
+  if (call == NULL_TREE
+      || TREE_CODE (call) != CALL_EXPR
+      || !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 +863,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 0000000000000..8d48ca5205782
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,62 @@
+/* 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));
+
+	/* 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));
+}
+
+/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/14/22 21:30, Zhao Wei Liew wrote:
> On 14/02/2022, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> +/* Returns true if EXPR is a reference to an implicit
>>> +   call to operator=(). */
>>> +static bool
>>> +is_assignment_overload_ref_p (tree expr)
>>> +{
>>> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
>>> +    return false;
>>
>> This will only warn about op= that returns a reference, which is not
>> required.
>>
> 
> Ah I understand now. I added some new test cases for non-reference return types
> and copied some code from extract_call_expr that seems to do what we want.

Good plan, but let's call extract_call_expr rather than copy from it.

>>> +  return fndecl != NULL_TREE
>>> +    && DECL_OVERLOADED_OPERATOR_P (fndecl)
>>> +    && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR)
>>> +    && DECL_ASSIGNMENT_OPERATOR_P (fndecl);
>>
>> This could be
>>
>>         && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
>>         && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
>>
>> without the separate DECL_OVERLOADED_OPERATOR_P test.
>>
> 
> I see. I've adjusted the code as you suggested and it works. Thanks!
> 
> ------ Everything below is the patch v4 ------
> 
> 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-pc-linux-gnu.
> 
> 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 (.
> 
> 	PR c/25689
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (maybe_convert_cond): Handle the implicit
> 	  operator=() case for -Wparentheses.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wparentheses-31.C: New test.
> ---
>   gcc/cp/semantics.cc                         | 29 +++++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
>   2 files changed, 90 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab1c..9c3f613a1aa62 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -815,6 +815,33 @@ 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
> +   to operator=() that is written as an operator expression. */
> +static bool
> +is_assignment_op_expr_p(tree call)

Missing space before (

> +{
> +  if (call == NULL_TREE)
> +    return false;
> +
> +  /* Unwrap the CALL_EXPR. */
> +  while (TREE_CODE (call) == COMPOUND_EXPR)
> +    call = TREE_OPERAND (call, 1);
> +  if (REFERENCE_REF_P (call))
> +    call = TREE_OPERAND (call, 0);
> +  if (TREE_CODE (call) == TARGET_EXPR)
> +    call = TARGET_EXPR_INITIAL (call);
> +
> +  if (call == NULL_TREE
> +      || TREE_CODE (call) != CALL_EXPR
> +      || !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 +863,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 0000000000000..8d48ca5205782
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,62 @@
> +/* 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));
> +
> +	/* 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));
> +}
> +
> +/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote:
>
> On 2/14/22 21:30, Zhao Wei Liew wrote:
> > On 14/02/2022, Jason Merrill <jason@redhat.com> wrote:
> >>>
> >>> +/* Returns true if EXPR is a reference to an implicit
> >>> +   call to operator=(). */
> >>> +static bool
> >>> +is_assignment_overload_ref_p (tree expr)
> >>> +{
> >>> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
> >>> +    return false;
> >>
> >> This will only warn about op= that returns a reference, which is not
> >> required.
> >>
> >
> > Ah I understand now. I added some new test cases for non-reference return types
> > and copied some code from extract_call_expr that seems to do what we want.
>
> Good plan, but let's call extract_call_expr rather than copy from it.
>

I agree. However, I can't seem to call extract_call_expr directly
because it calls gcc_assert
to assert that the resulting expression is a CALL_EXPR or AGGR_INIT_EXPR.
Instead, I've extracted the non-assert-related code into a
extract_call_expr_noassert function
and called that instead in the new patch. Is that okay?

> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -815,6 +815,33 @@ 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
> > +   to operator=() that is written as an operator expression. */
> > +static bool
> > +is_assignment_op_expr_p(tree call)
>
> Missing space before (
>

Thanks the catch. I've fixed that in the latest patch.

As always, thanks so much for your feedback!

------Everything below is v5 of the patch------

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.

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 (.

	PR c/25689

gcc/cp/ChangeLog:

	* call.cc (extract_call_expr): Refactor non-assert code to
	  extract_call_expr_noassert.
	(extract_call_expr_noassert): Refactor non-assert code.
	* cp-tree.h (extract_call_expr_noassert): Add prototype.
	* 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.
---
 gcc/cp/call.cc                              | 11 +++-
 gcc/cp/cp-tree.h                            |  1 +
 gcc/cp/semantics.cc                         | 22 +++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
 4 files changed, 94 insertions(+), 2 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 d6eed5ed83510..bead9d6b624c5 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7059,7 +7059,7 @@ build_op_subscript (const op_location_t &loc, tree obj,
    convert_from_reference.  */

 tree
-extract_call_expr (tree call)
+extract_call_expr_noassert (tree call)
 {
   while (TREE_CODE (call) == COMPOUND_EXPR)
     call = TREE_OPERAND (call, 1);
@@ -7090,6 +7090,15 @@ extract_call_expr (tree call)
       default:;
       }

+  return call;
+}
+
+/* Like extract_call_expr_noassert, but also asserts that
+   the extracted expression is indeed a call expression. */
+tree
+extract_call_expr (tree call)
+{
+  call = extract_call_expr_noassert (call);
   gcc_assert (TREE_CODE (call) == CALL_EXPR
 	      || TREE_CODE (call) == AGGR_INIT_EXPR
 	      || call == error_mark_node);
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index f253b32c3f21d..19a7a9d2c9334 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6519,6 +6519,7 @@ extern bool null_ptr_cst_p			(tree);
 extern bool null_member_pointer_value_p		(tree);
 extern bool sufficient_parms_p			(const_tree);
 extern tree type_decays_to			(tree);
+extern tree extract_call_expr_noassert		(tree);
 extern tree extract_call_expr			(tree);
 extern tree build_trivial_dtor_call		(tree, bool = false);
 extern bool ref_conv_binds_directly_p		(tree, tree);
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab1c..008e554273aa3 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_noassert (call);
+  if (call == NULL_TREE
+      || (TREE_CODE (call) != CALL_EXPR && TREE_CODE (call) != AGGR_INIT_EXPR)
+      || !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 0000000000000..8d48ca5205782
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,62 @@
+/* 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));
+
+	/* 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));
+}
+
+/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/15/22 05:06, Zhao Wei Liew wrote:
> On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 2/14/22 21:30, Zhao Wei Liew wrote:
>>> On 14/02/2022, Jason Merrill <jason@redhat.com> wrote:
>>>>>
>>>>> +/* Returns true if EXPR is a reference to an implicit
>>>>> +   call to operator=(). */
>>>>> +static bool
>>>>> +is_assignment_overload_ref_p (tree expr)
>>>>> +{
>>>>> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
>>>>> +    return false;
>>>>
>>>> This will only warn about op= that returns a reference, which is not
>>>> required.
>>>>
>>>
>>> Ah I understand now. I added some new test cases for non-reference return types
>>> and copied some code from extract_call_expr that seems to do what we want.
>>
>> Good plan, but let's call extract_call_expr rather than copy from it.
>>
> 
> I agree. However, I can't seem to call extract_call_expr directly
> because it calls gcc_assert
> to assert that the resulting expression is a CALL_EXPR or AGGR_INIT_EXPR.
> Instead, I've extracted the non-assert-related code into a
> extract_call_expr_noassert function
> and called that instead in the new patch. Is that okay?

I think instead of factoring out a new function, let's change the assert 
to an if and return NULL_TREE if it fails.

>>> --- a/gcc/cp/semantics.cc
>>> +++ b/gcc/cp/semantics.cc
>>> @@ -815,6 +815,33 @@ 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
>>> +   to operator=() that is written as an operator expression. */
>>> +static bool
>>> +is_assignment_op_expr_p(tree call)
>>
>> Missing space before (
>>
> 
> Thanks the catch. I've fixed that in the latest patch.
> 
> As always, thanks so much for your feedback!
> 
> ------Everything below is v5 of the patch------
> 
> 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.
> 
> 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 (.
> 
> 	PR c/25689
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (extract_call_expr): Refactor non-assert code to
> 	  extract_call_expr_noassert.
> 	(extract_call_expr_noassert): Refactor non-assert code.
> 	* cp-tree.h (extract_call_expr_noassert): Add prototype.
> 	* 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.
> ---
>   gcc/cp/call.cc                              | 11 +++-
>   gcc/cp/cp-tree.h                            |  1 +
>   gcc/cp/semantics.cc                         | 22 +++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
>   4 files changed, 94 insertions(+), 2 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 d6eed5ed83510..bead9d6b624c5 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -7059,7 +7059,7 @@ build_op_subscript (const op_location_t &loc, tree obj,
>      convert_from_reference.  */
> 
>   tree
> -extract_call_expr (tree call)
> +extract_call_expr_noassert (tree call)
>   {
>     while (TREE_CODE (call) == COMPOUND_EXPR)
>       call = TREE_OPERAND (call, 1);
> @@ -7090,6 +7090,15 @@ extract_call_expr (tree call)
>         default:;
>         }
> 
> +  return call;
> +}
> +
> +/* Like extract_call_expr_noassert, but also asserts that
> +   the extracted expression is indeed a call expression. */
> +tree
> +extract_call_expr (tree call)
> +{
> +  call = extract_call_expr_noassert (call);
>     gcc_assert (TREE_CODE (call) == CALL_EXPR
>   	      || TREE_CODE (call) == AGGR_INIT_EXPR
>   	      || call == error_mark_node);
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index f253b32c3f21d..19a7a9d2c9334 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6519,6 +6519,7 @@ extern bool null_ptr_cst_p			(tree);
>   extern bool null_member_pointer_value_p		(tree);
>   extern bool sufficient_parms_p			(const_tree);
>   extern tree type_decays_to			(tree);
> +extern tree extract_call_expr_noassert		(tree);
>   extern tree extract_call_expr			(tree);
>   extern tree build_trivial_dtor_call		(tree, bool = false);
>   extern bool ref_conv_binds_directly_p		(tree, tree);
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab1c..008e554273aa3 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_noassert (call);
> +  if (call == NULL_TREE
> +      || (TREE_CODE (call) != CALL_EXPR && TREE_CODE (call) != AGGR_INIT_EXPR)
> +      || !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 0000000000000..8d48ca5205782
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,62 @@
> +/* 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));
> +
> +	/* 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));
> +}
> +
> +/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/15/22 07:48, Jason Merrill wrote:
> On 2/15/22 05:06, Zhao Wei Liew wrote:
>> On Tue, 15 Feb 2022 at 13:13, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> On 2/14/22 21:30, Zhao Wei Liew wrote:
>>>> On 14/02/2022, Jason Merrill <jason@redhat.com> wrote:
>>>>>>
>>>>>> +/* Returns true if EXPR is a reference to an implicit
>>>>>> +   call to operator=(). */
>>>>>> +static bool
>>>>>> +is_assignment_overload_ref_p (tree expr)
>>>>>> +{
>>>>>> +  if (expr == NULL_TREE || !REFERENCE_REF_P (expr))
>>>>>> +    return false;
>>>>>
>>>>> This will only warn about op= that returns a reference, which is not
>>>>> required.
>>>>>
>>>>
>>>> Ah I understand now. I added some new test cases for non-reference 
>>>> return types
>>>> and copied some code from extract_call_expr that seems to do what we 
>>>> want.
>>>
>>> Good plan, but let's call extract_call_expr rather than copy from it.
>>>
>>
>> I agree. However, I can't seem to call extract_call_expr directly
>> because it calls gcc_assert
>> to assert that the resulting expression is a CALL_EXPR or AGGR_INIT_EXPR.
>> Instead, I've extracted the non-assert-related code into a
>> extract_call_expr_noassert function
>> and called that instead in the new patch. Is that okay?
> 
> I think instead of factoring out a new function, let's change the assert 
> to an if and return NULL_TREE if it fails.

Incidentally, the subject should be "c++:" instead of "c:".

Also, it doesn't look like you have a copyright assignment with the FSF, 
so you will need to add a DCO sign-off to your patches; see 
https://gcc.gnu.org/dco.html for more information.

I'd suggest putting your revision history before the scissors line, as 
that doesn't need to be part of the commit message.

And the latest patch didn't apply easily because this line:

 >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
 >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C

got wrapped in transit.

Jason

>>>> --- a/gcc/cp/semantics.cc
>>>> +++ b/gcc/cp/semantics.cc
>>>> @@ -815,6 +815,33 @@ 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
>>>> +   to operator=() that is written as an operator expression. */
>>>> +static bool
>>>> +is_assignment_op_expr_p(tree call)
>>>
>>> Missing space before (
>>>
>>
>> Thanks the catch. I've fixed that in the latest patch.
>>
>> As always, thanks so much for your feedback!
>>
>> ------Everything below is v5 of the patch------
>>
>> 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.
>>
>> 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 (.
>>
>>     PR c/25689
>>
>> gcc/cp/ChangeLog:
>>
>>     * call.cc (extract_call_expr): Refactor non-assert code to
>>       extract_call_expr_noassert.
>>     (extract_call_expr_noassert): Refactor non-assert code.
>>     * cp-tree.h (extract_call_expr_noassert): Add prototype.
>>     * 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.
>> ---
>>   gcc/cp/call.cc                              | 11 +++-
>>   gcc/cp/cp-tree.h                            |  1 +
>>   gcc/cp/semantics.cc                         | 22 +++++++-
>>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
>>   4 files changed, 94 insertions(+), 2 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 d6eed5ed83510..bead9d6b624c5 100644
>> --- a/gcc/cp/call.cc
>> +++ b/gcc/cp/call.cc
>> @@ -7059,7 +7059,7 @@ build_op_subscript (const op_location_t &loc, 
>> tree obj,
>>      convert_from_reference.  */
>>
>>   tree
>> -extract_call_expr (tree call)
>> +extract_call_expr_noassert (tree call)
>>   {
>>     while (TREE_CODE (call) == COMPOUND_EXPR)
>>       call = TREE_OPERAND (call, 1);
>> @@ -7090,6 +7090,15 @@ extract_call_expr (tree call)
>>         default:;
>>         }
>>
>> +  return call;
>> +}
>> +
>> +/* Like extract_call_expr_noassert, but also asserts that
>> +   the extracted expression is indeed a call expression. */
>> +tree
>> +extract_call_expr (tree call)
>> +{
>> +  call = extract_call_expr_noassert (call);
>>     gcc_assert (TREE_CODE (call) == CALL_EXPR
>>             || TREE_CODE (call) == AGGR_INIT_EXPR
>>             || call == error_mark_node);
>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>> index f253b32c3f21d..19a7a9d2c9334 100644
>> --- a/gcc/cp/cp-tree.h
>> +++ b/gcc/cp/cp-tree.h
>> @@ -6519,6 +6519,7 @@ extern bool null_ptr_cst_p            (tree);
>>   extern bool null_member_pointer_value_p        (tree);
>>   extern bool sufficient_parms_p            (const_tree);
>>   extern tree type_decays_to            (tree);
>> +extern tree extract_call_expr_noassert        (tree);
>>   extern tree extract_call_expr            (tree);
>>   extern tree build_trivial_dtor_call        (tree, bool = false);
>>   extern bool ref_conv_binds_directly_p        (tree, tree);
>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
>> index 0cb17a6a8ab1c..008e554273aa3 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_noassert (call);
>> +  if (call == NULL_TREE
>> +      || (TREE_CODE (call) != CALL_EXPR && TREE_CODE (call) != 
>> AGGR_INIT_EXPR)
>> +      || !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),
>> new file mode 100644
>> index 0000000000000..8d48ca5205782
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>> @@ -0,0 +1,62 @@
>> +/* 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));
>> +
>> +    /* 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));
>> +}
>> +
>> +/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@redhat.com> wrote:
> >>
> >> I agree. However, I can't seem to call extract_call_expr directly
> >> because it calls gcc_assert
> >> to assert that the resulting expression is a CALL_EXPR or
AGGR_INIT_EXPR.
> >> Instead, I've extracted the non-assert-related code into a
> >> extract_call_expr_noassert function
> >> and called that instead in the new patch. Is that okay?
> >
> > I think instead of factoring out a new function, let's change the assert
> > to an if and return NULL_TREE if it fails.
>

I've adjusted the patch as advised. What do you think?

> Incidentally, the subject should be "c++:" instead of "c:".
>

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.

> Also, it doesn't look like you have a copyright assignment with the FSF,
> so you will need to add a DCO sign-off to your patches; see
> https://gcc.gnu.org/dco.html for more information.
>
> I'd suggest putting your revision history before the scissors line, as
> that doesn't need to be part of the commit message.
>

Got it. Made the changes in the latest patch.

> And the latest patch didn't apply easily because this line:
>
>  >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>  >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>
> got wrapped in transit.
>

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.

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 (.


------ Everything below is patch v6 ------

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                              |  7 ++-
 gcc/cp/semantics.cc                         | 20 ++++++-
 gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
 3 files changed, 85 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..3b2c6d8c499 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;
 }

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab..7a8f317af0d 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,24 @@ 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_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 +854,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..8d48ca52057
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,62 @@
+/* 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));
+
+	/* 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));
+}
+
+/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

On Wed, 16 Feb 2022 at 00:30, Zhao Wei Liew <zhaoweiliew@gmail.com> wrote:

> On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@redhat.com> wrote:
> > >>
> > >> I agree. However, I can't seem to call extract_call_expr directly
> > >> because it calls gcc_assert
> > >> to assert that the resulting expression is a CALL_EXPR or
> AGGR_INIT_EXPR.
> > >> Instead, I've extracted the non-assert-related code into a
> > >> extract_call_expr_noassert function
> > >> and called that instead in the new patch. Is that okay?
> > >
> > > I think instead of factoring out a new function, let's change the
> assert
> > > to an if and return NULL_TREE if it fails.
> >
>
> I've adjusted the patch as advised. What do you think?
>
> > Incidentally, the subject should be "c++:" instead of "c:".
> >
>
> 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.
>
> > Also, it doesn't look like you have a copyright assignment with the FSF,
> > so you will need to add a DCO sign-off to your patches; see
> > https://gcc.gnu.org/dco.html for more information.
> >
> > I'd suggest putting your revision history before the scissors line, as
> > that doesn't need to be part of the commit message.
> >
>
> Got it. Made the changes in the latest patch.
>
> > And the latest patch didn't apply easily because this line:
> >
> >  >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> >  >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> >
> > got wrapped in transit.
> >
>
> 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.
>
> 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 (.
>
>
> ------ Everything below is patch v6 ------
>
> 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                              |  7 ++-
>  gcc/cp/semantics.cc                         | 20 ++++++-
>  gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
>  3 files changed, 85 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..3b2c6d8c499 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;
>  }
>
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab..7a8f317af0d 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -815,6 +815,24 @@ 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_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 +854,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
>
> Sorry, it seems like this line got wrapped again. I'll try my best to fix
it in future patches!

> new file mode 100644
> index 00000000000..8d48ca52057
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,62 @@
> +/* 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));
> +
> +	/* 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));
> +}
> +
> +/* 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] c: Add diagnostic when operator= is used as truth cond [PR25689]

[Jason Merrill]

On 2/15/22 11:30, Zhao Wei Liew wrote:
> On Tue, 15 Feb 2022 at 21:05, Jason Merrill <jason@redhat.com 
> <mailto:jason@redhat.com>> wrote:
>  > >>
>  > >> I agree. However, I can't seem to call extract_call_expr directly
>  > >> because it calls gcc_assert
>  > >> to assert that the resulting expression is a CALL_EXPR or 
> AGGR_INIT_EXPR.
>  > >> Instead, I've extracted the non-assert-related code into a
>  > >> extract_call_expr_noassert function
>  > >> and called that instead in the new patch. Is that okay?
>  > >
>  > > I think instead of factoring out a new function, let's change the 
> assert
>  > > to an if and return NULL_TREE if it fails.
>  >
> 
> I've adjusted the patch as advised. What do you think?
> 
>  > Incidentally, the subject should be "c++:" instead of "c:".
>  >
> 
> 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.

>  > Also, it doesn't look like you have a copyright assignment with the FSF,
>  > so you will need to add a DCO sign-off to your patches; see
>  > https://gcc.gnu.org/dco.html <https://gcc.gnu.org/dco.html> for more 
> information.
>  >
>  > I'd suggest putting your revision history before the scissors line, as
>  > that doesn't need to be part of the commit message.
>  >
> 
> Got it. Made the changes in the latest patch.
> 
>  > And the latest patch didn't apply easily because this line:
>  >
>  >  >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>  >  >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>  >
>  > got wrapped in transit.
>  >
> 
> 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.

> v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html 
> <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 
> <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 
> <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 
> <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 
> <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 (.
> 
> 
> ------ Everything below is patch v6 ------
> 
> 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  <mailto:zhaoweiliew@gmail.com>>
> ---
>   gcc/cp/call.cc                              |  7 ++-
>   gcc/cp/semantics.cc                         | 20 ++++++-
>   gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +++++++++++++++++++++
>   3 files changed, 85 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..3b2c6d8c499 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;

Note that since this can return error_mark_node...

>   }
>   
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab..7a8f317af0d 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -815,6 +815,24 @@ 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_EXPR_OPERATOR_SYNTAX (call))

...you probably want to check for it here.

> +    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 +854,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..8d48ca52057
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> @@ -0,0 +1,62 @@
> +/* 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));
> +
> +	/* 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.

> +}
> +
> +/* 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] c++: Add diagnostic when operator= is used as truth cond [PR25689]

[Zhao Wei Liew]

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

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.

> > -  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;
>
> Note that since this can return error_mark_node...
>
> >  }
> >  
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 0cb17a6a8ab..7a8f317af0d 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -815,6 +815,24 @@ 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_EXPR_OPERATOR_SYNTAX (call))
>
> ...you probably want to check for it here.

Good point. I've added the check.

> > +/* 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.


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