From: Zhao Wei Liew <zhaoweiliew@gmail.com>
To: Jason Merrill <jason@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]
Date: Mon, 14 Mar 2022 07:43:08 +0800 [thread overview]
Message-ID: <CALHvHFVRz1pc4zT7DnOs+ZkyhxmmttmnJ+rWQYaiBWhzbR37tw@mail.gmail.com> (raw)
In-Reply-To: <d374a027-7177-e3aa-20dd-7ce69696dd54@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]
On Sat, 12 Mar 2022 at 06:15, Jason Merrill <jason@redhat.com> wrote:
> It looks good, but unfortunately regresses some other warning tests,
> such as Wnonnull5.C. Please remember to run the regression tests before
> sending a patch (https://gcc.gnu.org/contribute.html#testing).
>
> This seems to be a complicated problem with suppress_warning, which
> means your call to suppress_warning effectively silences all later
> warnings, not just -Wparentheses.
>
> You should be able to work around this issue by only calling
> suppress_warning in the specific case we're interested in, i.e. when
> warn_parentheses is enabled and "call" is a MODIFY_EXPR.
My apologies. I've fixed the issue as you suggested and run the regression tests
to ensure no test regressions. The new patch (v9) is attached.
v8: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590570.html
Changes since v8:
1. Fix a test regression by calling suppress_warning only when "call"
is a MODIFY_EXPR.
v7: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590464.html
Changes since v7:
1. Suppress -Wparentheses warnings in build_new_method_call.
2. Uncomment the test case for if (b1.operator= (b2)).
v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
Changes since v6:
1. Check for error_mark_node in is_assignment_op_expr_pr.
2. Change "c:" to "c++:".
v5: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590393.html
Changes since v5:
1. Revert changes in v4.
2. Replace gcc_assert with a return NULL_TREE in extract_call_expr.
v4: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590379.html
Changes since v4:
1. Refactor the non-assert-related code out of extract_call_expr and
call that function instead to check for call expressions.
v3: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590310.html
Changes since v3:
1. Also handle COMPOUND_EXPRs and TARGET_EXPRs.
v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590236.html
Changes since v2:
1. Add more test cases in Wparentheses-31.C.
2. Refactor added logic to a function (is_assignment_overload_ref_p).
3. Use REFERENCE_REF_P instead of INDIRECT_REF_P.
v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590158.html
Changes since v1:
1. Use CALL_EXPR_OPERATOR_SYNTAX to avoid warnings for explicit
operator=() calls.
2. Use INDIRECT_REF_P to filter implicit operator=() calls.
3. Use cp_get_callee_fndecl_nofold.
4. Add spaces before (.
[-- Attachment #2: 0001-c-Add-diagnostic-when-operator-is-used-as-truth-cond.patch.txt --]
[-- Type: text/plain, Size: 5745 bytes --]
From 28f884d51a56889e84acba970a5aac9da8b24d99 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew <zhaoweiliew@gmail.com>
Date: Tue, 15 Feb 2022 17:44:29 +0800
Subject: [PATCH] c++: Add diagnostic when operator= is used as truth cond
[PR25689]
When compiling the following code with g++ -Wparentheses, GCC does not
warn on the if statement. For example, there is no warning for this code:
struct A {
A& operator=(int);
operator bool();
};
void f(A a) {
if (a = 0); // no warning
}
This is because a = 0 is a call to operator=, which GCC does not handle.
This patch fixes this issue by handling calls to operator= when deciding
to warn.
Bootstrapped and regression tested on x86_64-pc-linux-gnu.
PR c++/25689
gcc/cp/ChangeLog:
* call.cc (extract_call_expr): Return a NULL_TREE on failure
instead of asserting.
(build_new_method_call): Suppress -Wparentheses diagnostic for
MODIFY_EXPR.
* semantics.cc (is_assignment_op_expr_p): Add function to check
if an expression is a call to an op= operator expression.
(maybe_convert_cond): Handle the case of a op= operator expression
for the -Wparentheses diagnostic.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wparentheses-31.C: New test.
Signed-off-by: Zhao Wei Liew <zhaoweiliew@gmail.com>
---
gcc/cp/call.cc | 13 +++--
gcc/cp/semantics.cc | 22 +++++++-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 59 +++++++++++++++++++++
3 files changed, 90 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wparentheses-31.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 8fe8ef306ea..f502251c291 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7101,9 +7101,10 @@ extract_call_expr (tree call)
default:;
}
- gcc_assert (TREE_CODE (call) == CALL_EXPR
- || TREE_CODE (call) == AGGR_INIT_EXPR
- || call == error_mark_node);
+ if (TREE_CODE (call) != CALL_EXPR
+ && TREE_CODE (call) != AGGR_INIT_EXPR
+ && call != error_mark_node)
+ return NULL_TREE;
return call;
}
@@ -11148,6 +11149,12 @@ build_new_method_call (tree instance, tree fns, vec<tree, va_gc> **args,
*fn_p = fn;
/* Build the actual CALL_EXPR. */
call = build_over_call (cand, flags, complain);
+
+ /* Suppress warnings for if (my_struct.operator= (x)) where
+ my_struct is implicitly converted to bool. */
+ if (TREE_CODE (call) == MODIFY_EXPR)
+ suppress_warning (call, OPT_Wparentheses);
+
/* In an expression of the form `a->f()' where `f' turns
out to be a static member function, `a' is
none-the-less evaluated. */
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index d5565ebe02d..06e1db6e49a 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,26 @@ finish_goto_stmt (tree destination)
return add_stmt (build_stmt (input_location, GOTO_EXPR, destination));
}
+/* Returns true if CALL is a (possibly wrapped) CALL_EXPR or AGGR_INIT_EXPR
+ to operator= () that is written as an operator expression. */
+static bool
+is_assignment_op_expr_p (tree call)
+{
+ if (call == NULL_TREE)
+ return false;
+
+ call = extract_call_expr (call);
+ if (call == NULL_TREE
+ || call == error_mark_node
+ || !CALL_EXPR_OPERATOR_SYNTAX (call))
+ return false;
+
+ tree fndecl = cp_get_callee_fndecl_nofold (call);
+ return fndecl != NULL_TREE
+ && DECL_ASSIGNMENT_OPERATOR_P (fndecl)
+ && DECL_OVERLOADED_OPERATOR_IS (fndecl, NOP_EXPR);
+}
+
/* COND is the condition-expression for an if, while, etc.,
statement. Convert it to a boolean value, if appropriate.
In addition, verify sequence points if -Wsequence-point is enabled. */
@@ -836,7 +856,7 @@ maybe_convert_cond (tree cond)
/* Do the conversion. */
cond = convert_from_reference (cond);
- if (TREE_CODE (cond) == MODIFY_EXPR
+ if ((TREE_CODE (cond) == MODIFY_EXPR || is_assignment_op_expr_p (cond))
&& warn_parentheses
&& !warning_suppressed_p (cond, OPT_Wparentheses)
&& warning_at (cp_expr_loc_or_input_loc (cond),
diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
new file mode 100644
index 00000000000..6b5ce3c0e6b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
@@ -0,0 +1,59 @@
+/* Test that -Wparentheses warns for struct/class assignments,
+ except for explicit calls to operator= (). */
+/* PR c++/25689 */
+/* { dg-options "-Wparentheses" } */
+
+struct A
+{
+ A& operator= (int);
+ A operator= (double);
+ operator bool ();
+};
+
+struct B
+{
+ bool x;
+ B& operator= (int);
+ B operator= (double);
+ operator bool ();
+};
+
+struct C
+{
+ C& operator= (int);
+ virtual C operator= (double);
+ operator bool ();
+};
+
+/* Test empty class */
+void f1 (A a1, A a2)
+{
+ if (a1 = 0); /* { dg-warning "suggest parentheses" } */
+ if (a1 = 0.); /* { dg-warning "suggest parentheses" } */
+ if (a1.operator= (0));
+ if (a1.operator= (a2));
+
+ /* Ideally, we'd warn for empty classes using trivial operator= (below),
+ but we don't do so yet as it is a non-trivial COMPOUND_EXPR. */
+ // if (a1 = a2);
+}
+
+/* Test non-empty class */
+void f2 (B b1, B b2)
+{
+ if (b1 = 0); /* { dg-warning "suggest parentheses" } */
+ if (b1 = 0.); /* { dg-warning "suggest parentheses" } */
+ if (b1 = b2); /* { dg-warning "suggest parentheses" } */
+ if (b1.operator= (0));
+ if (b1.operator= (b2));
+}
+
+/* Test class with vtable */
+void f3 (C c1, C c2)
+{
+ if (c1 = 0); /* { dg-warning "suggest parentheses" } */
+ if (c1 = 0.); /* { dg-warning "suggest parentheses" } */
+ if (c1 = c2); /* { dg-warning "suggest parentheses" } */
+ if (c1.operator= (0));
+ if (c1.operator= (c2));
+}
--
2.25.1
next prev parent reply other threads:[~2022-03-13 23:43 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-16 7:16 Zhao Wei Liew
2022-02-16 16:59 ` Jason Merrill
2022-02-18 0:32 ` Zhao Wei Liew
2022-02-18 3:30 ` Zhao Wei Liew
2022-03-11 22:15 ` Jason Merrill
2022-03-13 23:43 ` Zhao Wei Liew [this message]
2022-03-24 22:12 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALHvHFVRz1pc4zT7DnOs+ZkyhxmmttmnJ+rWQYaiBWhzbR37tw@mail.gmail.com \
--to=zhaoweiliew@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).