On Fri, 11 Feb 2022 at 00:14, Jason Merrill <ja...@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
