On 2/12/22 01:59, Zhao Wei Liew wrote:
On Fri, 11 Feb 2022 at 20:47, Jason Merrill <ja...@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));
+}

Reply via email to