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

[Jason Merrill via Gcc-patches]

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 <ja...@redhat.com> wrote:

On 2/14/22 21:30, Zhao Wei Liew wrote:
On 14/02/2022, Jason Merrill <ja...@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