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?

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

Reply via email to