Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]
On Sat, 12 Mar 2022 at 06:15, Jason Merrill 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 (.
From 28f884d51a56889e84acba970a5aac9da8b24d99 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
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
---
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 **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_warnin
[PATCH] c++/96765: warn when casting "this" to Derived* in Base ctor/dtor
Hi!
This patch adds a warning when casting "this" to Derived* in the Base
class constructor and destructor. I've added the warning under the
-Wextra flag as I can't find a more appropriate flag for it.
The patch has been bootstrapped and regression tested on x86_64-pc-linux-gnu.
Appreciate reviews and feedback. Thanks!
From c70e087c7ee9db7497da293f8a85891abe895d9a Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
Date: Tue, 22 Feb 2022 16:03:17 +0800
Subject: [PATCH] c++: warn when casting "this" to Derived* in Base ctor/dtor
[PR96765]
Casting "this" in a base class constructor to a derived class type is
undefined behaviour, but there is no warning when doing so.
Add a warning for this.
Signed-off-by: Zhao Wei Liew
PR c++/96765
gcc/cp/ChangeLog:
* typeck.cc (build_static_cast_1): Add a warning when casting
"this" to Derived * in Base constructor and destructor.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wextra-5.C: New test.
---
gcc/cp/typeck.cc | 9
gcc/testsuite/g++.dg/warn/Wextra-5.C | 33
2 files changed, 42 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wextra-5.C
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 516fa574ef6..782f70b27e6 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -8079,6 +8079,15 @@ build_static_cast_1 (location_t loc, tree type, tree
expr, bool c_cast_p,
{
tree base;
+ if (current_function_decl
+ && (DECL_CONSTRUCTOR_P (current_function_decl)
+ || DECL_DESTRUCTOR_P (current_function_decl))
+ && TREE_CODE (expr) == NOP_EXPR
+ && is_this_parameter (TREE_OPERAND (expr, 0)))
+warning_at(loc, OPT_Wextra,
+ "invalid % from type %qT to type %qT before
the latter is constructed",
+ intype, type);
+
if (processing_template_decl)
return expr;
diff --git a/gcc/testsuite/g++.dg/warn/Wextra-5.C
b/gcc/testsuite/g++.dg/warn/Wextra-5.C
new file mode 100644
index 000..7e82c4c6121
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wextra-5.C
@@ -0,0 +1,33 @@
+// PR c++/96765
+// { dg-options "-Wextra" }
+
+struct Derived;
+struct Base {
+ Derived *x;
+ Derived *y;
+ Base();
+ ~Base();
+};
+
+struct Derived : Base {};
+
+Base::Base()
+: x(static_cast(this)), // { dg-warning "invalid 'static_cast'"
}
+ y((Derived *)this) // { dg-warning "invalid 'static_cast'" }
+{
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+Base::~Base() {
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+struct Other {
+ Other() {
+Base b;
+static_cast(&b);
+(Derived *)(&b);
+ }
+};
--
2.25.1
Re: [PATCH][v2] tree-optimization: Fold (type)X / (type)Y [PR103855]
Thanks for the detailed review. I have uploaded patch v2 based on the review. v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590604.html Changes since v1: 1. Add patterns for the cases CST / (T)x and (T)x / CST as well. Fix test regressions caused by those patterns. 2. Support signed integers except for the possible INT_MIN / -1 case. 3. Support cases where X and Y have different precisions. On Tue, 22 Feb 2022 at 15:54, Richard Biener wrote: > +/* (type)X / (type)Y -> (type)(X / Y) > + when the resulting type is at least precise as the original types > + and when all the types are unsigned integral types. */ > +(simplify > + (trunc_div (convert @0) (convert @1)) > + (if (INTEGRAL_TYPE_P (type) > + && INTEGRAL_TYPE_P (TREE_TYPE (@0)) > + && INTEGRAL_TYPE_P (TREE_TYPE (@1)) > + && TYPE_UNSIGNED (type) > + && TYPE_UNSIGNED (TREE_TYPE (@0)) > + && TYPE_UNSIGNED (TREE_TYPE (@1)) > + && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1)) > + && TYPE_PRECISION (type) >= TYPE_PRECISION (TREE_TYPE (@0))) > + (convert (trunc_div @0 @1 > > since you are requiring the types of @0 and @1 to match it's easier to write > > && types_match (TREE_TYPE(@0), TREE_TYPE (@1)) > Thanks. In the new patch, TREE_TYPE (@0) and TREE_TYPE (@1) can now have different precisions, so I believe that I can't use types_match() anymore. > that allows you to elide checks on either @0 or @1. I suppose the transform > does not work for signed types because of the -INT_MIN / -1 overflow case? > It might be possible to use expr_not_equal_to (@0, -INT_MIN) || > expr_not_equal_to (@1, -1) > (correctly written, lookup the existing examples in match.pd for the X > % -Y transform) That's right. I have made changes to support signed types except for the INT_MIN / -1 case. > I'll note that as written the transform will not catch CST / (T)x or > (T)x / CST since > you'll not see conversions around constants. I'm not sure whether > using (convert[12]? ...) > or writing special patterns with INTEGER_CST operands is more convenient. > There is int_fits_type_p to check whether a constant will fit in a > type without truncation > or sign change. I see. I couldn't find an easy way to use (convert[12]? ...) in this case so I added 2 other patterns for the CST / (T)x and (T)x / CST cases. > When @0 and @1 do not have the same type there might still be a common type > that can be used and is smaller than 'type', it might be as simple as using > build_nonstandard_integer_type (MIN (@0-prec, @1-prec), 1 /*unsigned_p*/). Thanks for the tip. I've made a similar change, but using either TREE_TYPE (@0) or TREE_TYPE (@1) instead of build_nonstandard_integer_type(). > > In the past there have been attempts to more globally narrow operations using > a new pass rather than using individual patterns. So for more complicated > cases > that might be the way to go. There's now also the ISEL pass which does > pre-RTL expansion transforms that need some global context and for example > can look at SSA uses. I had wanted to do something similar to handle all integral trunc_divs, but I'm not sure where/how to start. It seems out of my league at this moment. I'll gladly explore it after this change though! From f5f768b55f6cadcd9eba459561abfc1d7e28f94e Mon Sep 17 00:00:00 2001 From: Zhao Wei Liew Date: Sat, 19 Feb 2022 16:28:38 +0800 Subject: [PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y This pattern converts (trunc_div (convert X) (convert Y)) to (convert (trunc_div X Y)) when: 1. type, X, and Y are all INTEGRAL_TYPES with the same signedness. 2. type has TYPE_PRECISION at least as large as those of X and Y. 3. the result of the division is guaranteed to fit in either the type of Y or the type of Y. This patch also adds corresponding patterns for CST / (type)Y and (type)X / CST. These patterns useful as wider divisions are typically more expensive. Bootstrapped and regression tested on x86_64-pc-linux-gnu. Signed-off-by: Zhao Wei Liew PR tree-optimization/103855 gcc/ChangeLog: * match.pd: Add patterns for (type)X / (type)Y, CST / (type)Y, and (type)X / CST. gcc/testsuite/ChangeLog: * gcc.dg/ipa/pr91088.c: Fix a test regression. * gcc.dg/tree-ssa/divide-8.c: New test. --- gcc/match.pd | 57 gcc/testsuite/gcc.dg/ipa/pr91088.c | 4 +- gcc/testsuite/gcc.dg/tree-ssa/divide-8.c | 45 +++ 3 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-8.c diff --git a/gcc/match.pd b/gcc/match.pd index 8b44b5cc92c..b0bfb94f506 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -687,6 +687,63 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type)) (convert (trunc_mod @0 @1 +/* (type)X / (type)Y -> (type)(X / Y) + when all types are integral and have th
Re: [PATCH v2] c++/96765: warn when casting Base* to Derived* in Base ctor/dtor
Thanks for the review. I've tested and uploaded a new patch v2 with
the requested changes.
On Thu, 17 Mar 2022 at 09:20, Jason Merrill wrote:
>
> On 3/14/22 02:36, Zhao Wei Liew via Gcc-patches wrote:
>
> > This patch adds a warning when casting "this" to Derived* in the Base
> > class constructor and destructor. I've added the warning under the
> > -Wextra flag as I can't find a more appropriate flag for it.
>
> It's generally best to add a new warning flag if an existing one isn't a
> good match. Maybe -Wderived-cast-undefined? It seems like it could be
> part of -Wall.
Hmm, I agree. I've added a new -Wderived-cast-undefined flag for this.
> > + if (current_function_decl
> > + && (DECL_CONSTRUCTOR_P (current_function_decl)
> > + || DECL_DESTRUCTOR_P (current_function_decl))
> > + && TREE_CODE (expr) == NOP_EXPR
> > + && is_this_parameter (TREE_OPERAND (expr, 0)))
>
> I think the resolves_to_fixed_type_p function would be useful here;
> you're testing for a subset of the cases it handles.
Does the new patch look like how you intended it to? The new patch
passes all regression tests and newly added tests. However, I don't
fully understand the code for resolves_to_fixed_type_p() and
fixed_type_or_null(), except that I see that they contain logic to
return -1 when within a ctor/dtor.
Thanks!
From a8bbd4158f3311372b67d32816ce40d5613ae0d8 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
Date: Tue, 22 Feb 2022 16:03:17 +0800
Subject: [PATCH] c++: warn when casting Base* to Derived* in Base ctor/dtor
[PR96765]
Casting "this" in a base class constructor to a derived class type is
undefined behaviour, but there is no warning when doing so.
Add a warning for this.
Signed-off-by: Zhao Wei Liew
PR c++/96765
gcc/c-family/ChangeLog:
* c.opt (-Wderived-cast-undefined): New option.
gcc/cp/ChangeLog:
* typeck.cc (build_static_cast_1): Add a warning when casting
Base* to Derived* in Base constructor and destructor.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wderived-cast-undefined.C: New test.
---
gcc/c-family/c.opt| 4 +++
gcc/cp/typeck.cc | 6
.../g++.dg/warn/Wderived-cast-undefined.C | 33 +++
3 files changed, 43 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9cfd2a6bc4e..1681395c529 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -598,6 +598,10 @@ C++ ObjC++ Var(warn_deprecated_enum_float_conv) Warning
Warn about deprecated arithmetic conversions on operands where one is of
enumeration
type and the other is of a floating-point type.
+Wderived-cast-undefined
+C++ Var(warn_derived_cast_undefined) Warning LangEnabledBy(C++, Wall)
+Warn about undefined casts from Base* to Derived* in the Base constructor or
destructor.
+
Wdesignated-init
C ObjC Var(warn_designated_init) Init(1) Warning
Warn about positional initialization of structs requiring designated
initializers.
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 516fa574ef6..0bbbc378806 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -8079,6 +8079,12 @@ build_static_cast_1 (location_t loc, tree type, tree
expr, bool c_cast_p,
{
tree base;
+ if (TREE_CODE (expr) == NOP_EXPR
+ && resolves_to_fixed_type_p (TREE_OPERAND (expr, 0)) == -1)
+warning_at (loc, OPT_Wderived_cast_undefined,
+ "invalid % from type %qT to type %qT before
the latter is constructed",
+ intype, type);
+
if (processing_template_decl)
return expr;
diff --git a/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
b/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
new file mode 100644
index 000..ea7388514dc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
@@ -0,0 +1,33 @@
+// PR c++/96765
+// { dg-options "-Wderived-cast-undefined" }
+
+struct Derived;
+struct Base {
+ Derived *x;
+ Derived *y;
+ Base();
+ ~Base();
+};
+
+struct Derived : Base {};
+
+Base::Base()
+: x(static_cast(this)), // { dg-warning "invalid 'static_cast'"
}
+ y((Derived *)this) // { dg-warning "invalid 'static_cast'" }
+{
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+Base::~Base() {
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+struct Other {
+ Other() {
+Base b;
+static_cast(&b);
+(Derived *)(&b);
+ }
+};
--
2.25.1
Re: [PATCH v3] c++: warn on undefined casts from Base to Derived ref/ptr [PR96765]
On Fri, 25 Mar 2022 at 05:58, Jason Merrill wrote:
> >
> >>> + if (current_function_decl
> >>> + && (DECL_CONSTRUCTOR_P (current_function_decl)
> >>> + || DECL_DESTRUCTOR_P (current_function_decl))
> >>> + && TREE_CODE (expr) == NOP_EXPR
> >>> + && is_this_parameter (TREE_OPERAND (expr, 0)))
> >>
> >> I think the resolves_to_fixed_type_p function would be useful here;
> >> you're testing for a subset of the cases it handles.
> >
> > Does the new patch look like how you intended it to? The new patch
> > passes all regression tests and newly added tests. However, I don't
> > fully understand the code for resolves_to_fixed_type_p() and
> > fixed_type_or_null(), except that I see that they contain logic to
> > return -1 when within a ctor/dtor.
>
> > + if (TREE_CODE (expr) == NOP_EXPR
> > + && resolves_to_fixed_type_p (TREE_OPERAND (expr, 0)) == -1)
>
> Why not just pass expr itself to resolves_to_fixed_type_p?
You're right. I didn't realise that fixed_type_or_null handles the
NOP_EXPR case as part of CASE_CONVERT.
> We might as well also warn if it returns >0, e.g.
>
> struct A { } a;
> struct B: A { };
> auto p = static_cast(&a); // undefined
>
> You should also handle reference casts.
I've made some changes in v3 to handle reference casts and the case
where resolves_to_fixed_type_p > 0. I've also slightly modified the
patch title to reflect the new changes.
Thanks as always!
v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591894.html
Changes since v2:
- Handle reference casts and add new tests.
- Warn if resolves_to_fixed_type_p > 0 to handle more general cases.
v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591683.html
- Add new warning flag.
- Use resolves_to_fixed_type_p.
From 261fcff504d2a4971a501747979b1b424d6b09e4 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
Date: Tue, 22 Feb 2022 16:03:17 +0800
Subject: [PATCH] c++: warn on undefined casts from Base to Derived ref/ptr
[PR96765]
In some cases, converting a Base class reference/pointer to Derived
is undefined behavior.
This patch adds a new warning for them (-Wderived-cast-undefined).
Signed-off-by: Zhao Wei Liew
PR c++/96765
gcc/c-family/ChangeLog:
* c.opt (-Wderived-cast-undefined): New option.
gcc/cp/ChangeLog:
* typeck.cc (build_static_cast_1): Add warnings.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wderived-cast-undefined.C: New test.
---
gcc/c-family/c.opt| 4 ++
gcc/cp/typeck.cc | 13 +
.../g++.dg/warn/Wderived-cast-undefined.C | 49 +++
3 files changed, 66 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9a4828ebe37..1928aee62e6 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -598,6 +598,10 @@ C++ ObjC++ Var(warn_deprecated_enum_float_conv) Warning
Warn about deprecated arithmetic conversions on operands where one is of
enumeration
type and the other is of a floating-point type.
+Wderived-cast-undefined
+C++ Var(warn_derived_cast_undefined) Warning LangEnabledBy(C++, Wall)
+Warn about undefined casts from Base* to Derived* in the Base constructor or
destructor.
+
Wdesignated-init
C ObjC Var(warn_designated_init) Init(1) Warning
Warn about positional initialization of structs requiring designated
initializers.
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 516fa574ef6..6e19ba5af7f 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -7894,6 +7894,13 @@ build_static_cast_1 (location_t loc, tree type, tree
expr, bool c_cast_p,
{
tree base;
+ if (TYPE_MAIN_VARIANT (intype) != TYPE_MAIN_VARIANT (TREE_TYPE (type))
+ && resolves_to_fixed_type_p (expr))
+warning_at (loc, OPT_Wderived_cast_undefined,
+ "invalid % from type %qT to type %qT "
+ "before the latter is constructed",
+ intype, type);
+
if (processing_template_decl)
return expr;
@@ -8079,6 +8086,12 @@ build_static_cast_1 (location_t loc, tree type, tree
expr, bool c_cast_p,
{
tree base;
+ if (resolves_to_fixed_type_p (expr))
+warning_at (loc, OPT_Wderived_cast_undefined,
+ "invalid % from type %qT to type %qT "
+ "before the latter is constructed",
+ intype, type);
+
if (processing_template_decl)
return expr;
diff --git a/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
b/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
new file mode 100644
index 000..3459deadd3f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wderived-cast-undefined.C
@@ -0,0 +1,49 @@
+// PR c++/96765
+// { dg-options "-Wderived-cast-undefined" }
+
+struct Derived;
+struct Base {
+ Derived *x;
+ Derived &y;
+ Base();
+ ~Base();
+};
+
+struct Derived : Base {};
+
+Base::Bas
[PATCH v3] match.pd: Simplify 1 / X for integer X [PR95424]
This patch implements an optimization for the following C++ code:
int f(int x) {
return 1 / x;
}
int f(unsigned int x) {
return 1 / x;
}
Before this patch, x86-64 gcc -std=c++20 -O3 produces the following assembly:
f(int):
xor edx, edx
mov eax, 1
idiv edi
ret
f(unsigned int):
xor edx, edx
mov eax, 1
div edi
ret
In comparison, clang++ -std=c++20 -O3 produces the following assembly:
f(int):
lea ecx, [rdi + 1]
xor eax, eax
cmp ecx, 3
cmovb eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
Clang's output is more efficient as it avoids expensive div operations.
With this patch, GCC now produces the following assembly:
f(int):
lea eax, [rdi + 1]
cmp eax, 2
mov eax, 0
cmovbe eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
which is virtually identical to Clang's assembly output. Any slight differences
in the output for f(int) is possibly related to a different missed optimization.
v2: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587751.html
Changes from v2:
1. Refactor from using a switch statement to using the built-in
if-else statement.
v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587634.html
Changes from v1:
1. Refactor common if conditions.
2. Use build_[minus_]one_cst (type) to get -1/1 of the correct type.
3. Match only for TRUNC_DIV_EXPR and TYPE_PRECISION (type) > 1.
gcc/ChangeLog:
* match.pd: Simplify 1 / X where X is an integer.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/divide-6.c: New test.
* gcc.dg/tree-ssa/divide-7.c: New test.
---
gcc/match.pd | 13 +
gcc/testsuite/gcc.dg/tree-ssa/divide-6.c | 9 +
gcc/testsuite/gcc.dg/tree-ssa/divide-7.c | 9 +
3 files changed, 31 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 84c9b918041ee..4cd692c863d0c 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -432,6 +432,19 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& TYPE_UNSIGNED (type))
(trunc_div @0 @1)))
+ /* 1 / X -> X == 1 for unsigned integer X.
+1 / X -> X >= -1 && X <= 1 ? X : 0 for signed integer X.
+But not for 1 / 0 so that we can get proper warnings and errors,
+and not for 1-bit integers as they are edge cases better handled
elsewhere. */
+(simplify
+ (trunc_div integer_onep@0 @1)
+ (if (INTEGRAL_TYPE_P (type) && !integer_zerop (@1) &&
TYPE_PRECISION (type) > 1)
+(if (TYPE_UNSIGNED (type))
+ (eq @1 { build_one_cst (type); })
+ (with { tree utype = unsigned_type_for (type); }
+(cond (le (plus (convert:utype @1) { build_one_cst (utype);
}) { build_int_cst (utype, 2); })
+ @1 { build_zero_cst (type); })
+
/* Combine two successive divisions. Note that combining ceil_div
and floor_div is trickier and combining round_div even more so. */
(for div (trunc_div exact_div)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
new file mode 100644
index 0..a9fc4c04058c6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned int f(unsigned int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump "x_..D. == 1;" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
new file mode 100644
index 0..285279af7c210
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int f(int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump ".. <= 2 ? x_..D. : 0;" "optimized" } } */
--
2.17.1
Re: [PATCH] match.pd: Fix up 1 / X for unsigned X optimization [PR104280]
Sincere apologies for the issues. I wasn't aware of the need for a cast but after reading the PRs, I understand that now. On the other hand, the incorrect test case was simply a major oversight on my part. I'll be sure to be more careful next time. Thanks for the fixes!
[PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
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.
2. GCC does not warn for `if (a = b)` where the default copy/move
assignment operator is used.
I've included a code snippet in PR25689 that shows the 2 issues I
mentioned. I appreciate any feedback, thanks!
Everything below is the actual 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.
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 | 14 +-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 11 +++
2 files changed, 24 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..6a25d039585f2 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -836,7 +836,19 @@ maybe_convert_cond (tree cond)
/* Do the conversion. */
cond = convert_from_reference (cond);
- 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));
+ }
+
+ 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 0..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
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On Fri, 11 Feb 2022 at 00:14, Jason Merrill 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 0..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
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On Fri, 11 Feb 2022 at 20:47, Jason Merrill 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;
+
+ 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);
+}
+
/* 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 000..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" } */
+
+str
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On 14/02/2022, Jason Merrill 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.
>> + 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.
>
I see. I've adjusted the code as you suggested and it works. Thanks!
-- Everything below is the patch v4 --
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.
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:
* 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 | 29 +-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +
2 files changed, 90 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 0cb17a6a8ab1c..9c3f613a1aa62 100644
--- 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)
+{
+ if (call == NULL_TREE)
+return false;
+
+ /* Unwrap the CALL_EXPR. */
+ while (TREE_CODE (call) == COMPOUND_EXPR)
+call = TREE_OPERAND (call, 1);
+ if (REFERENCE_REF_P (call))
+call = TREE_OPERAND (call, 0);
+ if (TREE_CODE (call) == TARGET_EXPR)
+call = TARGET_EXPR_INITIAL (call);
+
+ if (call == NULL_TREE
+ || TREE_CODE (call) != CALL_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 +863,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 0..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);
+
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On Tue, 15 Feb 2022 at 13:13, Jason Merrill wrote:
>
> On 2/14/22 21:30, Zhao Wei Liew wrote:
> > On 14/02/2022, Jason Merrill 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 +
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On Tue, 15 Feb 2022 at 21:05, Jason Merrill wrote:
> >>
> >> 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?
> >
> > I think instead of factoring out a new function, let's change the assert
> > to an if and return NULL_TREE if it fails.
>
I've adjusted the patch as advised. What do you think?
> Incidentally, the subject should be "c++:" instead of "c:".
>
Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
subject with "c:",
but I just went with it as I didn't know any better. Unfortunately, I can't
change it now on
the current thread.
> Also, it doesn't look like you have a copyright assignment with the FSF,
> so you will need to add a DCO sign-off to your patches; see
> https://gcc.gnu.org/dco.html for more information.
>
> I'd suggest putting your revision history before the scissors line, as
> that doesn't need to be part of the commit message.
>
Got it. Made the changes in the latest patch.
> And the latest patch didn't apply easily because this line:
>
> >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
>
> got wrapped in transit.
>
Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
mailing list
setup so there are some kinks I have to iron out.
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 (.
-- Everything below is patch v6 --
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.
PR c++/25689
gcc/cp/ChangeLog:
* call.cc (extract_call_expr): Return a NULL_TREE on failure
instead of asserting.
* 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
---
gcc/cp/call.cc | 7 ++-
gcc/cp/semantics.cc | 20 ++-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +
3 files changed, 85 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 d6eed5ed835..3b2c6d8c499 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7090,9 +7090,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;
}
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 0cb17a6a8ab..7a8f317af0d 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -815,6 +815,24 @@ 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
Re: [PATCH] c: Add diagnostic when operator= is used as truth cond [PR25689]
On Wed, 16 Feb 2022 at 00:30, Zhao Wei Liew wrote:
> On Tue, 15 Feb 2022 at 21:05, Jason Merrill wrote:
> > >>
> > >> 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?
> > >
> > > I think instead of factoring out a new function, let's change the
> assert
> > > to an if and return NULL_TREE if it fails.
> >
>
> I've adjusted the patch as advised. What do you think?
>
> > Incidentally, the subject should be "c++:" instead of "c:".
> >
>
> Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> subject with "c:",
> but I just went with it as I didn't know any better. Unfortunately, I
> can't change it now on
> the current thread.
>
> > Also, it doesn't look like you have a copyright assignment with the FSF,
> > so you will need to add a DCO sign-off to your patches; see
> > https://gcc.gnu.org/dco.html for more information.
> >
> > I'd suggest putting your revision history before the scissors line, as
> > that doesn't need to be part of the commit message.
> >
>
> Got it. Made the changes in the latest patch.
>
> > And the latest patch didn't apply easily because this line:
> >
> > >> diff --git a/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> > >> b/gcc/testsuite/g++.dg/warn/Wparentheses-31.C
> >
> > got wrapped in transit.
> >
>
> Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> mailing list
> setup so there are some kinks I have to iron out.
>
> 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 (.
>
>
> -- Everything below is patch v6 --
>
> 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.
>
> PR c++/25689
>
> gcc/cp/ChangeLog:
>
> * call.cc (extract_call_expr): Return a NULL_TREE on failure
> instead of asserting.
> * 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
> ---
> gcc/cp/call.cc | 7 ++-
> gcc/cp/semantics.cc | 20 ++-
> gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +
> 3 files changed, 85 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 d6eed5ed835..3b2c6d8c499 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -7090,9 +7090,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;
> }
>
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 0cb17a6a8ab..7a8f317af0d 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -815,6 +815,24 @@ finish_goto_stmt (tree destination)
Re: [PATCH] c++: Add diagnostic when operator= is used as truth cond [PR25689]
On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> > Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> > subject with "c:",
> > but I just went with it as I didn't know any better. Unfortunately, I
> > can't change it now on the current thread.
>
> That came from this line in the testcase:
>
> > +/* PR c/25689 */
>
> The PR should be c++/25689. Also, sometimes the bugzilla component
> isn't the same as the area of the compiler you're changing; the latter
> is what you want in the patch subject, so that the right people know to
> review it.
Oh, I see. Thanks for the explanation. I've fixed the line.
> > Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> > mailing list setup so there are some kinks I have to iron out.
>
> FWIW it's often easier to send the patch as an attachment.
Alright, I'll send patches as attachments instead. I originally sent
them as text as it is easier to comment on them.
> > - 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;
>
> Note that since this can return error_mark_node...
>
> > }
> >
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 0cb17a6a8ab..7a8f317af0d 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -815,6 +815,24 @@ 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_EXPR_OPERATOR_SYNTAX (call))
>
> ...you probably want to check for it here.
Good point. I've added the check.
> > +/* 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));
>
> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> build_over_call.
Unfortunately, that also affects the warning for if (b1 = b2) just 5
lines above. Both expressions seem to generate the same tree structure.
0001-c-Add-diagnostic-when-operator-is-used-as-truth-cond.patch
Description: application/mbox
Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]
Before I start, sincere apologies for the email mishaps! I was setting up
an email client and somehow the emails I sent did not initially seem to
go through, but they actually did. You might have received several
duplicate emails as a result.
On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> > Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> > subject with "c:",
> > but I just went with it as I didn't know any better. Unfortunately, I
> > can't change it now on the current thread.
>
> That came from this line in the testcase:
>
> > +/* PR c/25689 */
>
> The PR should be c++/25689. Also, sometimes the bugzilla component
> isn't the same as the area of the compiler you're changing; the latter
> is what you want in the patch subject, so that the right people know to
> review it.
Oh, I see. Thanks for the explanation. I've fixed the line.
> > Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> > mailing list setup so there are some kinks I have to iron out.
>
> FWIW it's often easier to send the patch as an attachment.
Alright, I'll send patches as attachments instead. I originally sent
them as text as it is easier to comment on them.
> > +/* 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));
>
> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> build_over_call.
Unfortunately, that also affects the warning for if (b1 = b2) just 5
lines above. Both expressions seem to generate the same tree structure.
v6: https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590419.html
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 (.
From b89c3ebcfb4d7b3777bb70c1c0272cfd3bf29247 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
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 tested on x86_64-unknown-linux-gnu.
PR c++/25689
gcc/cp/ChangeLog:
* call.cc (extract_call_expr): Return a NULL_TREE on failure
instead of asserting.
* 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
---
gcc/cp/call.cc | 7 ++-
gcc/cp/semantics.cc | 22 +++-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 62 +
3 files changed, 87 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 d6eed5ed835..3b2c6d8c499 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7090,9 +7090,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
Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]
On Thu, 17 Feb 2022 at 00:59, Jason Merrill wrote:
>
> On 2/16/22 02:16, Zhao Wei Liew wrote:
> > On Wed Feb 16, 2022 at 4:06 AM +08, Jason Merrill wrote:
> >>> Ah, I see. I found it a bit odd that gcc-commit-mklog auto-generated a
> >>> subject with "c:",
> >>> but I just went with it as I didn't know any better. Unfortunately, I
> >>> can't change it now on the current thread.
> >>
> >> That came from this line in the testcase:
> >>
> >> > +/* PR c/25689 */
> >>
> >> The PR should be c++/25689. Also, sometimes the bugzilla component
> >> isn't the same as the area of the compiler you're changing; the latter
> >> is what you want in the patch subject, so that the right people know to
> >> review it.
> >
> > Oh, I see. Thanks for the explanation. I've fixed the line.
> >
> >>> Ah, I didn't notice that. Sorry about that! I'm kinda new to the whole
> >>> mailing list setup so there are some kinks I have to iron out.
> >>
> >> FWIW it's often easier to send the patch as an attachment.
> >
> > Alright, I'll send patches as attachments instead. I originally sent
> > them as text as it is easier to comment on them.
>
> It is a bit more of a hassle in this case because your mail sender
> doesn't mark the patch as text, but rather application/mbox or
> application/x-patch, so my mail reader for patch review (Thunderbird)
> doesn't display it inline. I tried sending myself a patch through the
> gmail web interface, and it used text/x-patch, which is OK; what are you
> using to send?
>
> Maybe renaming the file to .txt before sending would help?
Hmm, in the end I used gmail to send the patch, so I'm not sure why it was
marked that way. I'll test it out again before sending another patch.
> >>> +/* 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));
> >>
> >> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> >> build_over_call.
> >
> > Unfortunately, that also affects the warning for if (b1 = b2) just 5
> > lines above. Both expressions seem to generate the same tree structure.
>
> True, you would need to put the call to suppress_warning in build_new_op
> around where CALL_EXPR_OPERATOR_SYNTAX is set.
It seems like that would suppress the warning for the case of if (b1 = b2)
instead of
if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
in build_method_call instead?
This is what I've tried so far:
1. Call suppress_warning (result, ...) in the trivial_fn_p block in
build_new_op,
right above the comment "There won't be a CALL_EXPR" (line 6699).
This suppresses the warning for if (b1 = b2) but not for if
(b1.operator= (b2)).
2. Call suppress_warning (result, ...) in build_method_call, right after
the call to
build_over_call (line 11141). This suppresses the warning for if
(b1.operator= (b2))
and not if (b1 = b2).
Based on this, I think the 2nd option might be what we want here? Please
correct me if I'm
wrong. I'm also unsure if there are issues that might arise with this
change.
Re: [PATCH v7] c++: Add diagnostic when operator= is used as truth cond [PR25689]
On Fri, 18 Feb 2022 at 08:32, Zhao Wei Liew wrote:
>
> > >>> +/* 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));
> > >>
> > >> You can avoid it by calling suppress_warning on that MODIFY_EXPR in
> > >> build_over_call.
> > >
> > > Unfortunately, that also affects the warning for if (b1 = b2) just 5
> > > lines above. Both expressions seem to generate the same tree structure.
> >
> > True, you would need to put the call to suppress_warning in build_new_op
> > around where CALL_EXPR_OPERATOR_SYNTAX is set.
>
> It seems like that would suppress the warning for the case of if (b1 = b2)
> instead of
> if (b1.operator= (b2)). Do you mean to add the call to suppress_warning
> in build_method_call instead?
>
> This is what I've tried so far:
>
> 1. Call suppress_warning (result, ...) in the trivial_fn_p block in
> build_new_op,
>right above the comment "There won't be a CALL_EXPR" (line 6699).
>This suppresses the warning for if (b1 = b2) but not for if (b1.operator=
> (b2)).
>
> 2. Call suppress_warning (result, ...) in build_method_call, right after the
> call to
> build_over_call (line 11141). This suppresses the warning for if
> (b1.operator= (b2))
> and not if (b1 = b2).
>
> Based on this, I think the 2nd option might be what we want here? Please
> correct me if I'm
> wrong. I'm also unsure if there are issues that might arise with this change.
To better illustrate the 2nd option, I've attached it as a patch v8.
How does it look?
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 (.
From ef4cfecca64b2cb199a5d3979fe99f8c9bd0f414 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
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 tested on x86_64-unknown-linux-gnu.
PR c++/25689
gcc/cp/ChangeLog:
* call.cc (extract_call_expr): Return a NULL_TREE on failure
instead of asserting.
* 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
---
gcc/cp/call.cc | 12 +++--
gcc/cp/semantics.cc | 22 +++-
gcc/testsuite/g++.dg/warn/Wparentheses-31.C | 59 +
3 files changed, 89 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 d6eed5ed835..caf22e02b39 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -7090,9 +7090,10 @@ extract_call_expr
[PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y
This pattern converts (trunc_div (convert a) (convert b)) to
(convert (trunc_div a b)) when:
1. type, a, and b all have unsigned integeral types
2. a and b have the same type precision
3. type has type precision at least as larger as a and b
This is useful as wider divisions are typically more expensive.
To illustrate the effects, consider the following code snippet:
unsigned long long f(unsigned int a, unsigned int b) {
unsigned long long all = a;
return all / b;
}
Without the pattern, g++ -std=c++20 -O2 generates the following
assembly:
f(unsigned int, unsigned int):
mov eax, edi
mov esi, esi
xor edx, edx
div rsi
ret
With the pattern, it generates this:
f(unsigned int, unsigned int):
mov eax, edi
xor edx, edx
div esi
ret
This is identical to what clang++ -std=c++20 -O2 generates.
Signed-off-by: Zhao Wei Liew
PR tree-optimization/103855
gcc/ChangeLog:
* match.pd: Add pattern for (type)X / (type)Y.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/divide-8.c: New test.
* gcc.dg/tree-ssa/divide-9.c: New test.
---
gcc/match.pd | 15 +++
gcc/testsuite/gcc.dg/tree-ssa/divide-8.c | 9 +
gcc/testsuite/gcc.dg/tree-ssa/divide-9.c | 10 ++
3 files changed, 34 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 10f62284862..393b43756dd 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -684,6 +684,21 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(if (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
(convert (trunc_mod @0 @1
+/* (type)X / (type)Y -> (type)(X / Y)
+ when the resulting type is at least precise as the original types
+ and when all the types are unsigned integral types. */
+(simplify
+ (trunc_div (convert @0) (convert @1))
+ (if (INTEGRAL_TYPE_P (type)
+ && INTEGRAL_TYPE_P (TREE_TYPE (@0))
+ && INTEGRAL_TYPE_P (TREE_TYPE (@1))
+ && TYPE_UNSIGNED (type)
+ && TYPE_UNSIGNED (TREE_TYPE (@0))
+ && TYPE_UNSIGNED (TREE_TYPE (@1))
+ && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1))
+ && TYPE_PRECISION (type) >= TYPE_PRECISION (TREE_TYPE (@0)))
+ (convert (trunc_div @0 @1
+
/* x * (1 + y / x) - y -> x - y % x */
(simplify
(minus (mult:cs @0 (plus:s (trunc_div:s @1 @0) integer_onep)) @1)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
new file mode 100644
index 000..489604c4eb6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
@@ -0,0 +1,9 @@
+/* PR tree-optimization/103855 */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned int f(unsigned int a, unsigned int b) {
+unsigned long long all = a;
+return all / b;
+}
+
+/* { dg-final { scan-tree-dump-not "\(unsigned long long int)" "optimized" } }
*/
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
new file mode 100644
index 000..3e75a49b509
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
@@ -0,0 +1,10 @@
+/* PR tree-optimization/103855 */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned long long f(unsigned int a, unsigned int b) {
+unsigned long long all = a;
+return all / b;
+}
+
+/* { dg-final { scan-tree-dump-times "\\\(unsigned long long int\\\)" 1
"optimized" } } */
+
--
2.35.1
Re: [PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y
> On 19 Feb 2022, at 5:36 PM, Zhao Wei Liew wrote:
>
> This pattern converts (trunc_div (convert a) (convert b)) to
> (convert (trunc_div a b)) when:
>
> 1. type, a, and b all have unsigned integeral types
> 2. a and b have the same type precision
> 3. type has type precision at least as larger as a and b
>
> This is useful as wider divisions are typically more expensive.
>
> To illustrate the effects, consider the following code snippet:
>
> unsigned long long f(unsigned int a, unsigned int b) {
> unsigned long long all = a;
> return all / b;
> }
>
> Without the pattern, g++ -std=c++20 -O2 generates the following
> assembly:
>
> f(unsigned int, unsigned int):
> mov eax, edi
> mov esi, esi
> xor edx, edx
> div rsi
> ret
>
> With the pattern, it generates this:
>
> f(unsigned int, unsigned int):
> mov eax, edi
> xor edx, edx
> div esi
> ret
>
> This is identical to what clang++ -std=c++20 -O2 generates.
>
> Signed-off-by: Zhao Wei Liew
>
> PR tree-optimization/103855
>
> gcc/ChangeLog:
>
> * match.pd: Add pattern for (type)X / (type)Y.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/divide-8.c: New test.
> * gcc.dg/tree-ssa/divide-9.c: New test.
> ---
> gcc/match.pd | 15 +++
> gcc/testsuite/gcc.dg/tree-ssa/divide-8.c | 9 +
> gcc/testsuite/gcc.dg/tree-ssa/divide-9.c | 10 ++
> 3 files changed, 34 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 10f62284862..393b43756dd 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -684,6 +684,21 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> (if (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
> (convert (trunc_mod @0 @1
>
> +/* (type)X / (type)Y -> (type)(X / Y)
> + when the resulting type is at least precise as the original types
> + and when all the types are unsigned integral types. */
> +(simplify
> + (trunc_div (convert @0) (convert @1))
> + (if (INTEGRAL_TYPE_P (type)
> + && INTEGRAL_TYPE_P (TREE_TYPE (@0))
> + && INTEGRAL_TYPE_P (TREE_TYPE (@1))
> + && TYPE_UNSIGNED (type)
> + && TYPE_UNSIGNED (TREE_TYPE (@0))
> + && TYPE_UNSIGNED (TREE_TYPE (@1))
> + && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1))
> + && TYPE_PRECISION (type) >= TYPE_PRECISION (TREE_TYPE (@0)))
> + (convert (trunc_div @0 @1
> +
> /* x * (1 + y / x) - y -> x - y % x */
> (simplify
> (minus (mult:cs @0 (plus:s (trunc_div:s @1 @0) integer_onep)) @1)
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
> b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
> new file mode 100644
> index 000..489604c4eb6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
> @@ -0,0 +1,9 @@
> +/* PR tree-optimization/103855 */
> +/* { dg-options "-O -fdump-tree-optimized" } */
> +
> +unsigned int f(unsigned int a, unsigned int b) {
> +unsigned long long all = a;
> +return all / b;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "\(unsigned long long int)" "optimized" }
> } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
> b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
> new file mode 100644
> index 000..3e75a49b509
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/103855 */
> +/* { dg-options "-O -fdump-tree-optimized" } */
> +
> +unsigned long long f(unsigned int a, unsigned int b) {
> +unsigned long long all = a;
> +return all / b;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "\\\(unsigned long long int\\\)" 1
> "optimized" } } */
> +
> --
> 2.35.1
>
Sorry, I noticed issues with the test cases when running a regression test.
I’ll complete regression testing before uploading a v2.
[PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y
Hi, This is a partial optimization for PR103855. Initially, I looked into optimizing RTL generation or a more complex GIMPLE transformation so that we could optimize other cases as well, such as ((unsigned long long) short / int). However, that is a bit too complex for now. While I continue to look into that change, I've decided to implement this simpler match.pd transformation. Greatly appreciate any feedback on this patch or guidance for implementing the more advanced optimizations! Thanks, Zhao Wei 0001-tree-optimization-PR103855-Fold-type-X-type-Y.patch Description: Binary data
Re: [PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y
On Tue, 22 Feb 2022 at 11:57, Zhao Wei Liew wrote:
>
> Hi,
>
> This is a partial optimization for PR103855.
>
> Initially, I looked into optimizing RTL generation or a more complex
> GIMPLE transformation so that we could optimize other cases as well,
> such as ((unsigned long long) short / int).
>
> However, that is a bit too complex for now. While I continue to look
> into that change, I've decided to implement this simpler match.pd
> transformation.
>
> Greatly appreciate any feedback on this patch or guidance for
> implementing the more advanced optimizations!
>
> Thanks,
> Zhao Wei
Sorry, the original patch wasn't recognized as a text file. I've added
a .txt extension to make it explicit.
From dd3bb05cd7be72d080598cb693549ac74d5cb02d Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
Date: Sat, 19 Feb 2022 16:28:38 +0800
Subject: [PATCH] tree-optimization: [PR103855] Fold (type)X / (type)Y
This pattern converts (trunc_div (convert a) (convert b)) to
(convert (trunc_div a b)) when:
1. type, a, and b all have unsigned integeral types
2. a and b have the same type precision
3. type has type precision at least as larger as a and b
This is useful as wider divisions are typically more expensive.
To illustrate the effects, consider the following code snippet:
unsigned long long f(unsigned int a, unsigned int b) {
unsigned long long all = a;
return all / b;
}
Without the pattern, g++ -std=c++20 -O2 generates the following
assembly:
f(unsigned int, unsigned int):
mov eax, edi
mov esi, esi
xor edx, edx
div rsi
ret
With the pattern, it generates this:
f(unsigned int, unsigned int):
mov eax, edi
xor edx, edx
div esi
ret
This is identical to what clang++ -std=c++20 -O2 generates.
Bootstrapped and regression tested on x86_64-pc-linux-gnu.
Signed-off-by: Zhao Wei Liew
PR tree-optimization/103855
gcc/ChangeLog:
* match.pd: Add pattern for (type)X / (type)Y.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/divide-8.c: New test.
* gcc.dg/tree-ssa/divide-9.c: New test.
---
gcc/match.pd | 15 +++
gcc/testsuite/gcc.dg/tree-ssa/divide-8.c | 9 +
gcc/testsuite/gcc.dg/tree-ssa/divide-9.c | 9 +
3 files changed, 33 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 10f62284862..393b43756dd 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -684,6 +684,21 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(if (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
(convert (trunc_mod @0 @1
+/* (type)X / (type)Y -> (type)(X / Y)
+ when the resulting type is at least precise as the original types
+ and when all the types are unsigned integral types. */
+(simplify
+ (trunc_div (convert @0) (convert @1))
+ (if (INTEGRAL_TYPE_P (type)
+ && INTEGRAL_TYPE_P (TREE_TYPE (@0))
+ && INTEGRAL_TYPE_P (TREE_TYPE (@1))
+ && TYPE_UNSIGNED (type)
+ && TYPE_UNSIGNED (TREE_TYPE (@0))
+ && TYPE_UNSIGNED (TREE_TYPE (@1))
+ && TYPE_PRECISION (TREE_TYPE (@0)) == TYPE_PRECISION (TREE_TYPE (@1))
+ && TYPE_PRECISION (type) >= TYPE_PRECISION (TREE_TYPE (@0)))
+ (convert (trunc_div @0 @1
+
/* x * (1 + y / x) - y -> x - y % x */
(simplify
(minus (mult:cs @0 (plus:s (trunc_div:s @1 @0) integer_onep)) @1)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
new file mode 100644
index 000..dc3dc9ca769
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-8.c
@@ -0,0 +1,9 @@
+/* PR tree-optimization/103855 */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned int f(unsigned int a, unsigned int b) {
+unsigned long long all = a;
+return all / b;
+}
+
+/* { dg-final { scan-tree-dump-not "\\\(long long unsigned int\\\)"
"optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
new file mode 100644
index 000..6986b5484e4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-9.c
@@ -0,0 +1,9 @@
+/* PR tree-optimization/103855 */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned long long f(unsigned int a, unsigned int b) {
+unsigned long long all = a;
+return all / b;
+}
+
+/* { dg-final { scan-tree-dump-times "\\\(long long unsigned int\\\)" 1
"optimized" } } */
--
2.35.1
[PATCH][RFC] c++/96765: warn when casting Base* to Derived* in Base ctor/dtor
Hi!
This patch aims to add a warning when casting "this" in a base class
constructor to a derived class type. It works on the test cases
provided, but I'm still running regression tests.
However, I have a few doubts:
1. Am I missing out any cases? Right now, I'm identifying the casts by
checking that TREE_CODE (expr) == NOP_EXPR && is_this_parameter
(TREE_OPERAND (expr, 0)). It seems fine to me but perhaps there is a
function that I can use to express this more concisely?
2. -Wcast-qual doesn't seem to be the right flag for this warning.
However, I can't seem to find an appropriate flag. Maybe I should
place it under -Wextra or -Wall?
Appreciate any feedback on the aforementioned doubts or otherwise.
Thanks, and have a great day!
From 8a1f352f3db06faf264bc823387714a4a9e638b6 Mon Sep 17 00:00:00 2001
From: Zhao Wei Liew
Date: Tue, 22 Feb 2022 16:03:17 +0800
Subject: [PATCH] c++: warn on Base* to Derived* cast in Base ctor/dtor
[PR96765]
Casting "this" in a base class constructor to a derived class type is
undefined behaviour, but there is no warning when doing so.
Add a warning for this.
Signed-off-by: Zhao Wei Liew
PR c++/96765
gcc/cp/ChangeLog:
* typeck.cc (build_static_cast_1): Add a warning when casting
Base * to Derived * in Base constructor and destructor.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wcast-qual3.C: New test.
---
gcc/cp/typeck.cc| 8 ++
gcc/testsuite/g++.dg/warn/Wcast-qual3.C | 33 +
2 files changed, 41 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/warn/Wcast-qual3.C
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index f796337f73c..bbc40b25547 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -8080,6 +8080,14 @@ build_static_cast_1 (location_t loc, tree type, tree
expr, bool c_cast_p,
{
tree base;
+ if ((DECL_CONSTRUCTOR_P (current_function_decl)
+|| DECL_DESTRUCTOR_P (current_function_decl))
+ && TREE_CODE (expr) == NOP_EXPR
+ && is_this_parameter (TREE_OPERAND (expr, 0)))
+warning_at(loc, OPT_Wcast_qual,
+ "invalid % from type %qT to type %qT before
the latter is constructed",
+ intype, type);
+
if (processing_template_decl)
return expr;
diff --git a/gcc/testsuite/g++.dg/warn/Wcast-qual3.C
b/gcc/testsuite/g++.dg/warn/Wcast-qual3.C
new file mode 100644
index 000..8c44a23bd68
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wcast-qual3.C
@@ -0,0 +1,33 @@
+// PR c++/96765
+// { dg-options "-Wcast-qual" }
+
+struct Derived;
+struct Base {
+ Derived *x;
+ Derived *y;
+ Base();
+ ~Base();
+};
+
+struct Derived : Base {};
+
+Base::Base()
+: x(static_cast(this)), // { dg-warning "invalid 'static_cast'"
}
+ y((Derived *)this) // { dg-warning "invalid 'static_cast'" }
+{
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+Base::~Base() {
+ static_cast(this); // { dg-warning "invalid 'static_cast'" }
+ (Derived *)this; // { dg-warning "invalid 'static_cast'" }
+}
+
+struct Other {
+ Other() {
+Base b;
+static_cast(&b);
+(Derived *)(&b);
+ }
+};
--
2.35.1
[PATCH] match.pd: Simplify 1 / X for integer X [PR95424]
match.pd/95424: Simplify 1 / X for integer X
This patch implements an optimization for the following C++ code:
int f(int x) {
return 1 / x;
}
int f(unsigned int x) {
return 1 / x;
}
Before this patch, x86-64 gcc -std=c++20 -O3 produces the following
assembly:
f(int):
xor edx, edx
mov eax, 1
idiv edi
ret
f(unsigned int):
xor edx, edx
mov eax, 1
div edi
ret
In comparison, clang++ -std=c++20 -O3 produces the following assembly:
f(int):
lea ecx, [rdi + 1]
xor eax, eax
cmp ecx, 3
cmovb eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
Clang's output is more efficient as it avoids expensive div operations.
With this patch, GCC now produces the following assembly:
f(int):
lea eax, [rdi + 1]
cmp eax, 2
mov eax, 0
cmovbe eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
which is virtualy identical to Clang's assembly output. Any slight
differences
in the output for f(int) is related to another possible missed optimization.
gcc/ChangeLog:
* match.pd: Simplify 1 / X where X is an integer.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/divide-6.c: New test.
* gcc.dg/tree-ssa/divide-7.c: New test.
diff --git a/gcc/match.pd b/gcc/match.pd
index 84c9b918041..5edae1818bb 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -422,7 +422,24 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(div:C @0 (negate @0))
(if ((INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
&& TYPE_OVERFLOW_UNDEFINED (type))
-{ build_minus_one_cst (type); })))
+{ build_minus_one_cst (type); }))
+
+ /* 1 / X -> X == 1 for unsigned integer X
+1 / X -> X >= -1 && X <= 1 ? X : 0 for signed integer X
+But not for 1 / 0 so that we can get proper warnings and errors. */
+ (simplify
+ (div integer_onep@0 @1)
+ (switch
+ (if (!integer_zerop (@1)
+ && INTEGRAL_TYPE_P (TREE_TYPE (@1))
+ && TYPE_UNSIGNED (TREE_TYPE (@1)))
+ (eq @0 @1))
+ (if (!integer_zerop (@1)
+ && INTEGRAL_TYPE_P (TREE_TYPE (@1))
+ && !TYPE_UNSIGNED (TREE_TYPE (@1)))
+ (cond (bit_and (ge @1 { build_minus_one_cst (integer_type_node); })
+ (le @1 { build_one_cst (integer_type_node); }))
+@1 { build_zero_cst (type); })
/* For unsigned integral types, FLOOR_DIV_EXPR is the same as
TRUNC_DIV_EXPR. Rewrite into the latter in this case. */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
new file mode 100644
index 000..a9fc4c04058
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned int f(unsigned int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump "x_..D. == 1;" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
new file mode 100644
index 000..285279af7c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int f(int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump ".. <= 2 ? x_..D. : 0;" "optimized" } } */
Re: [PATCH] match.pd: Simplify 1 / X for integer X [PR95424]
> X >= -1 && X <= 1 is (hopefully?) going to be simplified
> as (unsigned)X + 1 <= 2, it might be good to simplify it this way
> here as well?
Yup, GCC does simplify it that way in the end, so I didn't really bother to
simplify it here. That said, I'm open to simplifying it here as well, but
I'm not sure how to do the unsigned cast.
Besides that, thanks for the rest of your suggestions! I'm testing the
changes and will post a v2 soon.
On Wed, 5 Jan 2022 at 16:18, Richard Biener
wrote:
>
> On Wed, Jan 5, 2022 at 7:15 AM Zhao Wei Liew via Gcc-patches
> wrote:
> >
> > match.pd/95424: Simplify 1 / X for integer X
> >
> > This patch implements an optimization for the following C++ code:
> >
> > int f(int x) {
> > return 1 / x;
> > }
> >
> > int f(unsigned int x) {
> > return 1 / x;
> > }
> >
> > Before this patch, x86-64 gcc -std=c++20 -O3 produces the following
> > assembly:
> >
> > f(int):
> > xor edx, edx
> > mov eax, 1
> > idiv edi
> > ret
> > f(unsigned int):
> > xor edx, edx
> > mov eax, 1
> > div edi
> > ret
> >
> > In comparison, clang++ -std=c++20 -O3 produces the following assembly:
> >
> > f(int):
> > lea ecx, [rdi + 1]
> > xor eax, eax
> > cmp ecx, 3
> > cmovb eax, edi
> > ret
> > f(unsigned int):
> > xor eax, eax
> > cmp edi, 1
> > sete al
> > ret
> >
> > Clang's output is more efficient as it avoids expensive div operations.
> >
> > With this patch, GCC now produces the following assembly:
> > f(int):
> > lea eax, [rdi + 1]
> > cmp eax, 2
> > mov eax, 0
> > cmovbe eax, edi
> > ret
> > f(unsigned int):
> > xor eax, eax
> > cmp edi, 1
> > sete al
> > ret
> >
> > which is virtualy identical to Clang's assembly output. Any slight
> > differences
> > in the output for f(int) is related to another possible missed
optimization.
> >
> > gcc/ChangeLog:
> >
> > * match.pd: Simplify 1 / X where X is an integer.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/tree-ssa/divide-6.c: New test.
> > * gcc.dg/tree-ssa/divide-7.c: New test.
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index 84c9b918041..5edae1818bb 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -422,7 +422,24 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > (div:C @0 (negate @0))
> > (if ((INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
> > && TYPE_OVERFLOW_UNDEFINED (type))
> > -{ build_minus_one_cst (type); })))
> > +{ build_minus_one_cst (type); }))
> > +
> > + /* 1 / X -> X == 1 for unsigned integer X
> > +1 / X -> X >= -1 && X <= 1 ? X : 0 for signed integer X
> > +But not for 1 / 0 so that we can get proper warnings and errors. */
> > + (simplify
> > + (div integer_onep@0 @1)
> > + (switch
> > + (if (!integer_zerop (@1)
> > + && INTEGRAL_TYPE_P (TREE_TYPE (@1))
>
> you can use INTEGRAL_TYPE_P (type), the type of @0, @1 and the
> result ('type') are the same.
>
> > + && TYPE_UNSIGNED (TREE_TYPE (@1)))
> > + (eq @0 @1))
> > + (if (!integer_zerop (@1)
> > + && INTEGRAL_TYPE_P (TREE_TYPE (@1))
> > + && !TYPE_UNSIGNED (TREE_TYPE (@1)))
>
> Please refactor as
>
> (if (INTEGRAL_TYPE_P (type)
> && !integer_zerop (@1))
> (if (TYPE_UNSIGNED ())
> (eq @0 @1)
> (cond (...
>
> > + (cond (bit_and (ge @1 { build_minus_one_cst (integer_type_node);
})
> > + (le @1 { build_one_cst (integer_type_node); }))
>
> You should use build_[minus_]one_cst (type) to get -1/1 of the correct
> type here.
>
> X >= -1 && X <= 1 is (hopefully?) going to be simplified
> as (unsigned)X + 1 <= 2, it might be good to simplify it this way
> here as well?
>
> Finally I'm not sure whether 1 /[ceil] 2 isn't 1, similar -1 /[floor]
> 2 should be -1
> so the transform looks to be correct only for TRUNC_DIV_EXPR, not all
'div'?
>
> Thanks,
> Richard.
>
> > +@1 { build_zero_cst (type); })
> >
> > /* For unsigned integral types, FLOOR_DIV_EXPR is the same as
> > TRUNC_DIV_EXPR. Rewrite into the latter in this case. */
> &
Re: [PATCH] match.pd: Simplify 1 / X for integer X [PR95424]
On Wed, 5 Jan 2022 at 17:55, Richard Biener
wrote:
> On Wed, Jan 5, 2022 at 10:42 AM Jakub Jelinek wrote:
> >
> > On Wed, Jan 05, 2022 at 10:38:53AM +0100, Richard Biener via Gcc-patches
> wrote:
> > > On Wed, Jan 5, 2022 at 10:18 AM Zhao Wei Liew
> wrote:
> > > >
> > > > > X >= -1 && X <= 1 is (hopefully?) going to be simplified
> > > > > as (unsigned)X + 1 <= 2, it might be good to simplify it this way
> > > > > here as well?
> > > >
> > > > Yup, GCC does simplify it that way in the end, so I didn't really
> bother to simplify it here. That said, I'm open to simplifying it here as
> well, but I'm not sure how to do the unsigned cast.
> > >
> > > You'd do sth like
> > > (with
> > > { tree utype = unsigned_type_for (type); }
> > > (cond (le (plus (convert:utype @0) { build_one_cst (utype); }) {
> > > build_int_cst (utype, 2); }) ...)
> > >
> > > extra tricky will be 1 bit integer types, I guess it might be easiest
> > > to exclude them
> > > and special case them separately - X / Y is always X for them I think,
> >
> > Note, we already have:
> > /* X / bool_range_Y is X. */
> > (simplify
> > (div @0 SSA_NAME@1)
> > (if (INTEGRAL_TYPE_P (type) && ssa_name_has_boolean_range (@1))
> >@0))
> > for those.
>
> Ah, it might not handle the signed : 1 case though since -1 is not in the
> bool range. We could generalize the above though.
>
> Richard.
>
> >
> > Jakub
> >
>
Perhaps it is possible to exclude 1-bit cases and rely on other existing
simplifications to deal with them?
In particular, GCC seems to already optimally simplify the signed and
unsigned 1-bit cases [1].
[1]:
struct S {
unsigned int x: 1;
int y: 1;
};
unsigned int fu(S s) {
return 1 / s.x;
}
int fi(S s) {
return 1 / s.y;
}
fu(S):
mov eax, 1
ret
fi(S):
mov eax, -1
ret
[PATCH v2] match.pd: Simplify 1 / X for integer X [PR95424]
This patch implements an optimization for the following C++ code:
int f(int x) {
return 1 / x;
}
int f(unsigned int x) {
return 1 / x;
}
Before this patch, x86-64 gcc -std=c++20 -O3 produces the following
assembly:
f(int):
xor edx, edx
mov eax, 1
idiv edi
ret
f(unsigned int):
xor edx, edx
mov eax, 1
div edi
ret
In comparison, clang++ -std=c++20 -O3 produces the following assembly:
f(int):
lea ecx, [rdi + 1]
xor eax, eax
cmp ecx, 3
cmovb eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
Clang's output is more efficient as it avoids expensive div operations.
With this patch, GCC now produces the following assembly:
f(int):
lea eax, [rdi + 1]
cmp eax, 2
mov eax, 0
cmovbe eax, edi
ret
f(unsigned int):
xor eax, eax
cmp edi, 1
sete al
ret
which is virtually identical to Clang's assembly output. Any slight
differences
in the output for f(int) is possibly related to a different missed
optimization.
v1: https://gcc.gnu.org/pipermail/gcc-patches/2022-January/587634.html
Changes from v1:
1. Refactor common if conditions.
2. Use build_[minus_]one_cst (type) to get -1/1 of the correct type.
3. Match only for TRUNC_DIV_EXPR and TYPE_PRECISION (type) > 1.
gcc/ChangeLog:
* match.pd: Simplify 1 / X where X is an integer.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/divide-6.c: New test.
* gcc.dg/tree-ssa/divide-7.c: New test.
---
gcc/match.pd | 15 +++
gcc/testsuite/gcc.dg/tree-ssa/divide-6.c | 9 +
gcc/testsuite/gcc.dg/tree-ssa/divide-7.c | 9 +
3 files changed, 33 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 84c9b918041..52a0f77f455 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -432,6 +432,21 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& TYPE_UNSIGNED (type))
(trunc_div @0 @1)))
+ /* 1 / X -> X == 1 for unsigned integer X.
+1 / X -> X >= -1 && X <= 1 ? X : 0 for signed integer X.
+But not for 1 / 0 so that we can get proper warnings and errors,
+and not for 1-bit integers as they are edge cases better handled
elsewhere. */
+(simplify
+ (trunc_div integer_onep@0 @1)
+ (if (INTEGRAL_TYPE_P (type) && !integer_zerop (@1) && TYPE_PRECISION
(type) > 1)
+(switch
+ (if (TYPE_UNSIGNED (type))
+(eq @1 { build_one_cst (type); }))
+ (if (!TYPE_UNSIGNED (type))
+(with { tree utype = unsigned_type_for (type); }
+ (cond (le (plus (convert:utype @1) { build_one_cst (utype); }) {
build_int_cst (utype, 2); })
+@1 { build_zero_cst (type); }))
+
/* Combine two successive divisions. Note that combining ceil_div
and floor_div is trickier and combining round_div even more so. */
(for div (trunc_div exact_div)
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
new file mode 100644
index 000..a9fc4c04058
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-6.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+unsigned int f(unsigned int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump "x_..D. == 1;" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
new file mode 100644
index 000..285279af7c2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/divide-7.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int f(int x) {
+ return 1 / x;
+}
+
+/* { dg-final { scan-tree-dump-not "1 / x_..D.;" "optimized" } } */
+/* { dg-final { scan-tree-dump ".. <= 2 ? x_..D. : 0;" "optimized" } } */
--
2.17.1
