Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=. [wwwdocs]
On 5/10/20, Eric Gallager wrote: > On 1/10/20, Jason Merrill wrote: >> Back in 2009 Manuel sent a patch to avoid useless -Wconversion warnings >> on compound assignment of types that get promoted to int: >> >> https://gcc.gnu.org/ml/gcc-patches/2009-08/msg00582.html >> >> Joseph argued that those warnings are sometimes useful, and that they >> should be controlled by a separate flag. So this patch introduces >> -Warith-conversion, which is off by default in this patch. >> >> Joseph, is that default OK with you? If not, can you explain your >> reasoning more about why the warnings are desirable? It seems to me >> that even in cases where you don't know the ranges involved, it's wrong >> for e.g. 'mychar += 1' to warn when 'myint += 1' doesn't. In both >> cases, the addition might overflow the range of the target type, but >> that isn't about the conversion; the result is the same as if the >> operation had been done in the operand/target type rather than the >> promoted type. >> >> The change to cp/typeck.c is a placeholder to use if the default setting >> changes; even if we end up warning by default for mychar = mychar + 1, I >> really don't want to warn about mychar += 1. >> >> sign.diff is a prerequisite tidying that moves the warnings from >> unsafe_conversion_p back into conversion_warning with the others. >> >> rshift.diff restores the short_shift code to the C++ front end so that C >> and C++ give the same warnings with -Warith-conversion. >> >> Tested x86_64-pc-linux-gnu. Comments? >> > > Hi Jason, thanks for this new flag. Now that GCC 10 has been released > I was checking the release notes to review everything new in GCC 10, > and I didn't see any mention of -Warith-conversion on it. Could you > add a quick note about -Warith-conversion to changes.html please? > > Thanks, > Eric > Something just like "Some warnings that -Wconversion previously issued now also require -Warith-conversion" or something would be fine.
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On 1/10/20, Jason Merrill wrote: > Back in 2009 Manuel sent a patch to avoid useless -Wconversion warnings > on compound assignment of types that get promoted to int: > > https://gcc.gnu.org/ml/gcc-patches/2009-08/msg00582.html > > Joseph argued that those warnings are sometimes useful, and that they > should be controlled by a separate flag. So this patch introduces > -Warith-conversion, which is off by default in this patch. > > Joseph, is that default OK with you? If not, can you explain your > reasoning more about why the warnings are desirable? It seems to me > that even in cases where you don't know the ranges involved, it's wrong > for e.g. 'mychar += 1' to warn when 'myint += 1' doesn't. In both > cases, the addition might overflow the range of the target type, but > that isn't about the conversion; the result is the same as if the > operation had been done in the operand/target type rather than the > promoted type. > > The change to cp/typeck.c is a placeholder to use if the default setting > changes; even if we end up warning by default for mychar = mychar + 1, I > really don't want to warn about mychar += 1. > > sign.diff is a prerequisite tidying that moves the warnings from > unsafe_conversion_p back into conversion_warning with the others. > > rshift.diff restores the short_shift code to the C++ front end so that C > and C++ give the same warnings with -Warith-conversion. > > Tested x86_64-pc-linux-gnu. Comments? > Hi Jason, thanks for this new flag. Now that GCC 10 has been released I was checking the release notes to review everything new in GCC 10, and I didn't see any mention of -Warith-conversion on it. Could you add a quick note about -Warith-conversion to changes.html please? Thanks, Eric
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On Jan 24, 2020, at 9:45 AM, David Edelsohn wrote: > > On Fri, Jan 24, 2020 at 12:00 PM Jason Merrill wrote: >> >> On 1/24/20 8:45 AM, David Edelsohn wrote: >>> There is no ChangeLog entry for the testsuite changes. >> >> I don't believe in ChangeLog entries for testcases, but I'll add one for >> the target-supports.exp change, thanks. > > Is this a general policy change that we want to make? Current we > still have gcc/testsuite/ChangeLog and developers are updating that > file. I kinda like the current policy. Use it if you want to, but, you can't count on others to use it. I'd say this is a slight preference of mine. Personally, I find git log , to be easier, faster, more accurate, can't go wrong. With that, I'd usually only go to ChangeLog for commentary, though, since the changelog entry should be into the git log entry, git log is still sufficient to get at the commentary. The only reason why git log works in this case, is because a single test case is in a single file with usually no or very little intermixing. This isn't true of expression.c for example. In the git log for expr.c, you'd have to wade through a ton of chaff, which makes it much harder to use in some situations. For the testsuite/lib/* files, I actually prefer ChangeLog entries (to match the policy of gcc/*.c). This isn't policy, but, I'd encourage others in this direction. I suppose I like ChangeLogs for all the *.exp files as well, as usually these are for a large multitude of test cases. That's just my $0.02. If others want to chime in, on what the like and why...
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On Fri, Jan 24, 2020 at 12:46 PM David Edelsohn wrote: > On Fri, Jan 24, 2020 at 12:00 PM Jason Merrill wrote: > > > > On 1/24/20 8:45 AM, David Edelsohn wrote: > > > There is no ChangeLog entry for the testsuite changes. > > > > I don't believe in ChangeLog entries for testcases, but I'll add one for > > the target-supports.exp change, thanks. > > Is this a general policy change that we want to make? Current we > still have gcc/testsuite/ChangeLog and developers are updating that > file. > I would support formalizing that as policy; currently there is no policy. https://gcc.gnu.org/codingconventions.html#ChangeLogs "There is no established convention on when ChangeLog entries are to be made for testsuite changes." Jason
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On Fri, Jan 24, 2020 at 12:00 PM Jason Merrill wrote: > > On 1/24/20 8:45 AM, David Edelsohn wrote: > > There is no ChangeLog entry for the testsuite changes. > > I don't believe in ChangeLog entries for testcases, but I'll add one for > the target-supports.exp change, thanks. Is this a general policy change that we want to make? Current we still have gcc/testsuite/ChangeLog and developers are updating that file. Thanks, David
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On 1/24/20 8:45 AM, David Edelsohn wrote: There is no ChangeLog entry for the testsuite changes. I don't believe in ChangeLog entries for testcases, but I'll add one for the target-supports.exp change, thanks. I'm also still trying to determine if Wconversion-pr40752.c requires -fsigned-char option. It shouldn't. Jason
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
Jason, There is no ChangeLog entry for the testsuite changes. I'm also still trying to determine if Wconversion-pr40752.c requires -fsigned-char option. Thanks, David
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On 1/22/20 5:58 PM, Jason Merrill wrote:
On 1/22/20 1:20 PM, Jason Merrill wrote:
On 1/22/20 5:14 AM, Martin Liška wrote:
Hi.
Just for the record, after the change 526.blender_r fails due to:
blender/source/blender/blenlib/intern/math_color.c: In function
'rgb_float_to_uchar':
blender/source/blender/blenlib/BLI_utildefines.h:251:22: error:
conversion from 'float' to 'unsigned char' may change value
[-Werror=float-conversion]
251 | #define FTOCHAR(val) ((CHECK_TYPE_INLINE(val, float)), \
| ^
blender/source/blender/blenlib/BLI_utildefines.h:257:13: note: in
expansion of macro 'FTOCHAR'
257 | (v1)[0] =
FTOCHAR((v2[0])); \
| ^~~
blender/source/blender/blenlib/intern/math_color.c:421:2: note: in
expansion of macro 'F3TOCHAR3'
421 | F3TOCHAR3(col_f, r_col);
| ^
where the project sets -Werror=float-conversion dues to:
$ cat blender/source/blender/blenlib/BLI_strict_flags.h
#ifdef __GNUC__
# if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */
# pragma GCC diagnostic error "-Wsign-compare"
# pragma GCC diagnostic error "-Wconversion"
# endif
Thanks, investigating.
I wasn't able to reproduce this particular error, but I saw a different
one due to -funsigned-char, fixed thus:
One more tweak, tested powerpc64-linux-gnu.
commit 608199de99de8e4f64e92bdf8b96564d2adeae5e
Author: Jason Merrill
Date: Thu Jan 23 10:37:18 2020 -0500
c-family: One more 40752 tweak for unsigned char.
My last patch didn't fix all the failures on unsignd char targets. We were
missing one warning because by suppressing -Wsign-conversion for the second
operand of + we missed an overflow that we want to warn about, and we
properly don't warn about unsigned / or %.
PR testsuite/93391 - PR 40752 test fails with unsigned plain char.
* c-warn.c (conversion_warning): Change -Wsign-conversion handling.
* lib/target-supports.exp (check_effective_target_unsigned_char):
New.
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 6c73317b4a6..9315cac683e 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -1315,10 +1315,14 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
for (int i = 0; i < arith_ops; ++i)
{
tree op = TREE_OPERAND (expr, i);
- tree opr = convert (type, op);
/* Avoid -Wsign-conversion for (unsigned)(x + (-1)). */
- bool minus = TREE_CODE (expr) == PLUS_EXPR && i == 1;
- if (unsafe_conversion_p (type, op, opr, !minus))
+ if (TREE_CODE (expr) == PLUS_EXPR && i == 1
+ && INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type)
+ && TREE_CODE (op) == INTEGER_CST
+ && tree_int_cst_sgn (op) < 0)
+ op = fold_build1 (NEGATE_EXPR, TREE_TYPE (op), op);
+ tree opr = convert (type, op);
+ if (unsafe_conversion_p (type, op, opr, true))
goto op_unsafe;
}
/* The operands seem safe, we might still want to warn if
diff --git a/gcc/testsuite/c-c++-common/Wconversion-pr40752a.c b/gcc/testsuite/c-c++-common/Wconversion-pr40752a.c
index cd70e34c390..8e3ffae06f6 100644
--- a/gcc/testsuite/c-c++-common/Wconversion-pr40752a.c
+++ b/gcc/testsuite/c-c++-common/Wconversion-pr40752a.c
@@ -14,9 +14,10 @@ void foo(char c, char c2)
c *= 2; /* { dg-warning "conversion" } */
c *= c2; /* { dg-warning "conversion" } */
c /= 2;
- c /= c2; /* { dg-warning "conversion" } */
+ /* If char is unsigned we avoid promoting to int. */
+ c /= c2; /* { dg-warning "conversion" "" { target { ! unsigned_char } } } */
c %= 2;
- c %= c2; /* { dg-warning "conversion" } */
+ c %= c2; /* { dg-warning "conversion" "" { target { ! unsigned_char } } } */
c = -c2; /* { dg-warning "conversion" } */
c = ~c2; /* { dg-warning "conversion" } */
c = c2++;
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index f65a8100da9..3ca3dd3a9e4 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -3115,6 +3115,14 @@ proc check_effective_target_dfprt { } {
}]
}
+# Return 1 iff target has unsigned plain 'char' by default.
+
+proc check_effective_target_unsigned_char {} {
+return [check_no_compiler_messages unsigned_char assembly {
+ char ar[(char)-1];
+}]
+}
+
proc check_effective_target_powerpc_popcntb_ok { } {
return [check_cached_effective_target powerpc_popcntb_ok {
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On 1/22/20 1:20 PM, Jason Merrill wrote:
On 1/22/20 5:14 AM, Martin Liška wrote:
Hi.
Just for the record, after the change 526.blender_r fails due to:
blender/source/blender/blenlib/intern/math_color.c: In function
'rgb_float_to_uchar':
blender/source/blender/blenlib/BLI_utildefines.h:251:22: error:
conversion from 'float' to 'unsigned char' may change value
[-Werror=float-conversion]
251 | #define FTOCHAR(val) ((CHECK_TYPE_INLINE(val, float)), \
| ^
blender/source/blender/blenlib/BLI_utildefines.h:257:13: note: in
expansion of macro 'FTOCHAR'
257 | (v1)[0] =
FTOCHAR((v2[0])); \
| ^~~
blender/source/blender/blenlib/intern/math_color.c:421:2: note: in
expansion of macro 'F3TOCHAR3'
421 | F3TOCHAR3(col_f, r_col);
| ^
where the project sets -Werror=float-conversion dues to:
$ cat blender/source/blender/blenlib/BLI_strict_flags.h
#ifdef __GNUC__
# if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */
# pragma GCC diagnostic error "-Wsign-compare"
# pragma GCC diagnostic error "-Wconversion"
# endif
Thanks, investigating.
I wasn't able to reproduce this particular error, but I saw a different
one due to -funsigned-char, fixed thus:
commit 55b7df8bfb12938e7716445d4e2dc0d2ddf44bac
Author: Jason Merrill
Date: Wed Jan 22 14:21:06 2020 -0500
c-family: Fix problems with blender and PPC from PR 40752 patch.
blender in SPEC is built with -funsigned-char, which is also the default on
PPC, and exposed -Wsign-conversion issues that weren't seen by the x86_64
testsuite. In blender we were complaining about operands to an expression
that we didn't't previously complain about as a whole. So only check
operands after we check the whole expression. Also, to fix the PR 40752
testcases on -funsigned-char targets, don't consider -Wsign-conversion for
the second operand of PLUS_EXPR, especially since fold changes
"x - 5" to "x + (-5)". And don't use SCHAR_MAX with plain char.
PR testsuite/93391 - PR 40752 test fails with unsigned plain char.
PR c++/40752
* c-warn.c (conversion_warning): Check operands only after checking
the whole expression. Don't check second operand of + for sign.
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 4df4893ca02..6c73317b4a6 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -1163,7 +1163,7 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
{
tree expr_type = TREE_TYPE (expr);
enum conversion_safety conversion_kind;
- bool is_arith = false;
+ int arith_ops = 0;
if (!warn_conversion && !warn_sign_conversion && !warn_float_conversion)
return false;
@@ -1266,14 +1266,8 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
case CEIL_DIV_EXPR:
case EXACT_DIV_EXPR:
case RDIV_EXPR:
- {
- tree op0 = TREE_OPERAND (expr, 0);
- tree op1 = TREE_OPERAND (expr, 1);
- if (conversion_warning (loc, type, op0, result)
- || conversion_warning (loc, type, op1, result))
- return true;
- goto arith_op;
- }
+ arith_ops = 2;
+ goto default_;
case PREDECREMENT_EXPR:
case PREINCREMENT_EXPR:
@@ -1285,13 +1279,8 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
case NON_LVALUE_EXPR:
case NEGATE_EXPR:
case BIT_NOT_EXPR:
- {
- /* Unary ops or binary ops for which we only care about the lhs. */
- tree op0 = TREE_OPERAND (expr, 0);
- if (conversion_warning (loc, type, op0, result))
- return true;
- goto arith_op;
- }
+ arith_ops = 1;
+ goto default_;
case COND_EXPR:
{
@@ -1304,11 +1293,7 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
|| conversion_warning (loc, type, op2, result));
}
-arith_op:
- /* We didn't warn about the operands, we might still want to warn if
- -Warith-conversion. */
- is_arith = true;
- gcc_fallthrough ();
+default_:
default:
conversion_kind = unsafe_conversion_p (type, expr, result, true);
{
@@ -1321,11 +1306,27 @@ conversion_warning (location_t loc, tree type, tree expr, tree result)
warnopt = OPT_Wconversion;
else
break;
- if (is_arith
+
+ if (arith_ops
&& global_dc->option_enabled (warnopt,
global_dc->lang_mask,
global_dc->option_state))
- warnopt = OPT_Warith_conversion;
+ {
+ for (int i = 0; i < arith_ops; ++i)
+ {
+ tree op = TREE_OPERAND (expr, i);
+ tree opr = convert (type, op);
+ /* Avoid -Wsign-conversion for (unsigned)(x + (-1)). */
+ bool minus = TREE_CODE (expr) == PLUS_EXPR && i == 1;
+ if (unsafe_conversion_p (type, op, opr, !minus))
+ goto op_unsafe;
+ }
+ /* The operands seem safe, we might still want to warn if
+ -Warith-conversion. */
+ warnopt =
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On 1/22/20 5:14 AM, Martin Liška wrote: Hi. Just for the record, after the change 526.blender_r fails due to: blender/source/blender/blenlib/intern/math_color.c: In function 'rgb_float_to_uchar': blender/source/blender/blenlib/BLI_utildefines.h:251:22: error: conversion from 'float' to 'unsigned char' may change value [-Werror=float-conversion] 251 | #define FTOCHAR(val) ((CHECK_TYPE_INLINE(val, float)), \ | ^ blender/source/blender/blenlib/BLI_utildefines.h:257:13: note: in expansion of macro 'FTOCHAR' 257 | (v1)[0] = FTOCHAR((v2[0])); \ | ^~~ blender/source/blender/blenlib/intern/math_color.c:421:2: note: in expansion of macro 'F3TOCHAR3' 421 | F3TOCHAR3(col_f, r_col); | ^ where the project sets -Werror=float-conversion dues to: $ cat blender/source/blender/blenlib/BLI_strict_flags.h #ifdef __GNUC__ # if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */ # pragma GCC diagnostic error "-Wsign-compare" # pragma GCC diagnostic error "-Wconversion" # endif Thanks, investigating. Jason
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
Hi. Just for the record, after the change 526.blender_r fails due to: blender/source/blender/blenlib/intern/math_color.c: In function 'rgb_float_to_uchar': blender/source/blender/blenlib/BLI_utildefines.h:251:22: error: conversion from 'float' to 'unsigned char' may change value [-Werror=float-conversion] 251 | #define FTOCHAR(val) ((CHECK_TYPE_INLINE(val, float)), \ | ^ blender/source/blender/blenlib/BLI_utildefines.h:257:13: note: in expansion of macro 'FTOCHAR' 257 | (v1)[0] = FTOCHAR((v2[0])); \ | ^~~ blender/source/blender/blenlib/intern/math_color.c:421:2: note: in expansion of macro 'F3TOCHAR3' 421 | F3TOCHAR3(col_f, r_col); | ^ where the project sets -Werror=float-conversion dues to: $ cat blender/source/blender/blenlib/BLI_strict_flags.h #ifdef __GNUC__ # if (__GNUC__ * 100 + __GNUC_MINOR__) >= 406 /* gcc4.6+ only */ #pragma GCC diagnostic error "-Wsign-compare" #pragma GCC diagnostic error "-Wconversion" # endif Martin
Re: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
On Fri, 10 Jan 2020, Jason Merrill wrote: > Joseph argued that those warnings are sometimes useful, and that they should > be controlled by a separate flag. So this patch introduces > -Warith-conversion, which is off by default in this patch. > > Joseph, is that default OK with you? I am OK with that default. -- Joseph S. Myers jos...@codesourcery.com
RE: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
Hi Jason, The commit Shorten right-shift again in C++. Back in SVN r131862 richi removed this code to fix PR 34235, but didn't remove the parallel code from the C front-end because the bug had previously been fixed in r44080. This patch copies the code from C again. * typeck.c (cp_build_binary_op): Restore short_shift code. From-SVN: r280128 Is causing ICEs in SPEC2017's leela. internal compiler error: tree check: expected integer_cst, have mult_expr in to_wide, at tree.h:5860 368 | int allblack = ((m_neighbours[i] >> (NBR_SHIFT * BLACK)) & 7) == 4; | ^ 0x6565d3 tree_check_failed(tree_node const*, char const*, int, char const*, ...) ../.././gcc/tree.c:9685 0x11d3d33 tree_int_cst_elt_check(tree_node*, int, char const*, int, char const*) ../.././gcc/tree.h:3478 0x11d3d33 tree_int_cst_elt_check(tree_node const*, int, char const*, int, char const*) ../.././gcc/tree.h:3474 0x11d3d33 wi::to_wide(tree_node const*) ../.././gcc/tree.h:5860 0x11d3d33 tree_int_cst_sgn(tree_node const*) ../.././gcc/tree.c:7382 0x90ad8f cp_build_binary_op(op_location_t const&, tree_code, tree_node*, tree_node*, int) ../.././gcc/cp/typeck.c:5609 0x681547 build_new_op_1 ../.././gcc/cp/call.c:6475 0x681fdf build_new_op(op_location_t const&, tree_code, int, tree_node*, tree_node*, tree_node*, tree_node**, int) ../.././gcc/cp/call.c:6520 0x8f960f build_x_binary_op(op_location_t const&, tree_code, tree_node*, tree_code, tree_node*, tree_code, tree_node**, int) ../.././gcc/cp/typeck.c:4245 0x7dc7cf cp_parser_binary_expression ../.././gcc/cp/parser.c:9648 0x7ddb1f cp_parser_assignment_expression ../.././gcc/cp/parser.c:9783 0x7dde3b cp_parser_expression ../.././gcc/cp/parser.c:9951 0x7f095f cp_parser_primary_expression ../.././gcc/cp/parser.c:5351 0x7fdefb cp_parser_postfix_expression ../.././gcc/cp/parser.c:7203 0x7dc607 cp_parser_binary_expression ../.././gcc/cp/parser.c:9483 0x7ddb1f cp_parser_assignment_expression ../.././gcc/cp/parser.c:9783 0x7dde3b cp_parser_expression ../.././gcc/cp/parser.c:9951 0x7f095f cp_parser_primary_expression ../.././gcc/cp/parser.c:5351 0x7fdefb cp_parser_postfix_expression ../.././gcc/cp/parser.c:7203 0x7dc607 cp_parser_binary_expression ../.././gcc/cp/parser.c:9483 Thanks, Tamar > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org > On Behalf Of Jason Merrill > Sent: Friday, January 10, 2020 6:15 PM > To: gcc-patches List ; Joseph S. Myers > > Cc: Manuel López-Ibáñez > Subject: [RFC c-common PATCH] PR c++/40752 - useless -Wconversion with > short +=. > > Back in 2009 Manuel sent a patch to avoid useless -Wconversion warnings on > compound assignment of types that get promoted to int: > > https://gcc.gnu.org/ml/gcc-patches/2009-08/msg00582.html > > Joseph argued that those warnings are sometimes useful, and that they > should be controlled by a separate flag. So this patch introduces -Warith- > conversion, which is off by default in this patch. > > Joseph, is that default OK with you? If not, can you explain your reasoning > more about why the warnings are desirable? It seems to me that even in > cases where you don't know the ranges involved, it's wrong for e.g. 'mychar > += 1' to warn when 'myint += 1' doesn't. In both cases, the addition might > overflow the range of the target type, but that isn't about the conversion; > the result is the same as if the operation had been done in the > operand/target type rather than the promoted type. > > The change to cp/typeck.c is a placeholder to use if the default setting > changes; even if we end up warning by default for mychar = mychar + 1, I > really don't want to warn about mychar += 1. > > sign.diff is a prerequisite tidying that moves the warnings from > unsafe_conversion_p back into conversion_warning with the others. > > rshift.diff restores the short_shift code to the C++ front end so that C and > C++ give the same warnings with -Warith-conversion. > > Tested x86_64-pc-linux-gnu. Comments?
[RFC c-common PATCH] PR c++/40752 - useless -Wconversion with short +=.
Back in 2009 Manuel sent a patch to avoid useless -Wconversion warnings
on compound assignment of types that get promoted to int:
https://gcc.gnu.org/ml/gcc-patches/2009-08/msg00582.html
Joseph argued that those warnings are sometimes useful, and that they
should be controlled by a separate flag. So this patch introduces
-Warith-conversion, which is off by default in this patch.
Joseph, is that default OK with you? If not, can you explain your
reasoning more about why the warnings are desirable? It seems to me
that even in cases where you don't know the ranges involved, it's wrong
for e.g. 'mychar += 1' to warn when 'myint += 1' doesn't. In both
cases, the addition might overflow the range of the target type, but
that isn't about the conversion; the result is the same as if the
operation had been done in the operand/target type rather than the
promoted type.
The change to cp/typeck.c is a placeholder to use if the default setting
changes; even if we end up warning by default for mychar = mychar + 1, I
really don't want to warn about mychar += 1.
sign.diff is a prerequisite tidying that moves the warnings from
unsafe_conversion_p back into conversion_warning with the others.
rshift.diff restores the short_shift code to the C++ front end so that C
and C++ give the same warnings with -Warith-conversion.
Tested x86_64-pc-linux-gnu. Comments?
commit 77b31408e3de8178f20972744d06501adb6ea961
Author: Jason Merrill
Date: Tue Jan 7 12:20:26 2020 -0500
PR c++/40752 - useless -Wconversion with short +=.
This is a longstanding issue with lots of duplicates; people are not
interested in a -Wconversion warning about mychar += 1. So now that warning
depends on -Warith-conversion; otherwise we only warn if operands of the
arithmetic have conversion issues.
* c.opt (-Warith-conversion): New.
* c-warn.c (conversion_warning): Recurse for operands of
operators. Only warn about the whole expression with
-Warith-conversion.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 53df4b1fdf9..d4ab699ccc7 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -291,7 +291,8 @@ Objective-C and Objective-C++ Dialects}.
-Waggregate-return -Waligned-new @gol
-Walloc-zero -Walloc-size-larger-than=@var{byte-size} @gol
-Walloca -Walloca-larger-than=@var{byte-size} @gol
--Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
+-Wno-aggressive-loop-optimizations -Warith-conversion @gol
+-Warray-bounds -Warray-bounds=@var{n} @gol
-Wno-attributes -Wattribute-alias=@var{n} @gol
-Wbool-compare -Wbool-operation @gol
-Wno-builtin-declaration-mismatch @gol
@@ -6244,6 +6245,24 @@ See also @option{-Wvla-larger-than=}@samp{byte-size}.
Disable @option{-Walloca-larger-than=} warnings. The option is
equivalent to @option{-Walloca-larger-than=}@samp{SIZE_MAX} or larger.
+@item -Warith-conversion
+@opindex Warith-conversion
+@opindex Wno-arith-conversion
+Do warn about implicit conversions from arithmetic operations even
+when conversion of the operands to the same type cannot change their
+values. This affects warnings from @option{-Wconversion},
+@option{-Wfloat-conversion}, and @option{-Wsign-conversion}.
+
+@smallexample
+@group
+void f (char c, int i)
+@{
+ c = c + i; // warns with @option{-Wconversion}
+ c = c + 1; // only warns with @option{-Warith-conversion}
+@}
+@end group
+@end smallexample
+
@item -Warray-bounds
@itemx -Warray-bounds=@var{n}
@opindex Wno-array-bounds
@@ -7032,6 +7051,9 @@ reference to them. Warnings about conversions between signed and
unsigned integers are disabled by default in C++ unless
@option{-Wsign-conversion} is explicitly enabled.
+Warnings about conversion from arithmetic on a small type back to that
+type are only given with @option{-Warith-conversion}.
+
@item -Wno-conversion-null @r{(C++ and Objective-C++ only)}
@opindex Wconversion-null
@opindex Wno-conversion-null
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 230b76387ba..88bc4f443e2 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1107,6 +1107,10 @@ Wshift-negative-value
C ObjC C++ ObjC++ Var(warn_shift_negative_value) Init(-1) Warning
Warn if left shifting a negative value.
+Warith-conversion
+C ObjC C++ ObjC++ Var(warn_arith_conv) Warning
+Warn if conversion of the result of arithmetic might change the value even though converting the operands cannot.
+
Wsign-compare
C ObjC C++ ObjC++ Var(warn_sign_compare) Warning LangEnabledBy(C++ ObjC++,Wall)
Warn about signed-unsigned comparisons.
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index 6dbc660ddb4..d8f0ad654fe 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -1155,17 +1155,18 @@ check_main_parameter_types (tree decl)
"%q+D declared as variadic function", decl);
}
-/* Warns if the conversion of EXPR to TYPE may alter a value.
+/* Warns and returns true
