On 2/10/22 23:01, Zhao Wei Liew wrote:
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?
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.
- 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?
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=?
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.
------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)) {
The { should go on the next line.
+ 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
