Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

On Thu, Aug 10, 2017 at 09:35:07AM +0200, Andreas Schwab wrote:
> FAIL: gcc.dg/compare2.c case 20 (test for warnings, line 49)
> FAIL: gcc.dg/compare2.c case 24 (test for warnings, line 57)
> FAIL: gcc.dg/compare2.c (test for excess errors)
> Excess errors:
> /usr/local/gcc/gcc-20170810/gcc/testsuite/gcc.dg/compare2.c:49:12: warning: 
> operand of ?: changes signedness from 'int' to 'unsigned int' due to 
> unsignedness of other operand [-Wsign-compare]
> /usr/local/gcc/gcc-20170810/gcc/testsuite/gcc.dg/compare2.c:57:12: warning: 
> operand of ?: changes signedness from 'int' to 'unsigned int' due to 
> unsignedness of other operand [-Wsign-compare]

Fixed.

Marek

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Andreas Schwab]

FAIL: gcc.dg/compare2.c case 20 (test for warnings, line 49)
FAIL: gcc.dg/compare2.c case 24 (test for warnings, line 57)
FAIL: gcc.dg/compare2.c (test for excess errors)
Excess errors:
/usr/local/gcc/gcc-20170810/gcc/testsuite/gcc.dg/compare2.c:49:12: warning: 
operand of ?: changes signedness from 'int' to 'unsigned int' due to 
unsignedness of other operand [-Wsign-compare]
/usr/local/gcc/gcc-20170810/gcc/testsuite/gcc.dg/compare2.c:57:12: warning: 
operand of ?: changes signedness from 'int' to 'unsigned int' due to 
unsignedness of other operand [-Wsign-compare]

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Joseph Myers]

On Wed, 2 Aug 2017, Marek Polacek wrote:

> Hmm, how about this, then?
> 
> "operand of ?: changes signedness from %qT to %qT due to unsignedness of 
> other operand"
> 
> I couldn't come up with anything more brief yet conveying all the information.
> I don't like adding "second"/"third"/... very much; we should offer a good
> location already.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

OK.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

Ping.

The make_location change should allow us to do really cool things, if we
pass c_exprs to build_binary_loc, btw.  Like all the ranges for diagnostic
and similar.

On Wed, Aug 02, 2017 at 02:43:39PM +0200, Marek Polacek wrote:
> On Tue, Aug 01, 2017 at 04:48:01PM -0400, David Malcolm wrote:
> > I'm wondering if the messages could use a slight rewording, to give a
> > clue to the user about the reason *why* the expression has changed
> > signedness.  The old message "signed and unsigned type in conditional
> > expression" gave the clue (but failed to underline the subexpression
> > changing sign, and tell what the old/new types were).
> > 
> > A horribly verbose way to put it would be something like:
> > 
> > "operand of conditional expression with mixed signedness changes
> > signedness from %qT to %qT due to promotion to unsigned to match
> > unsignedness of other operand" (ugh)
> > 
> > (assuming I'm understanding the logic correctly)
> > 
> > or something like:
> > 
> > "operand of conditional expression changes signedness from %qT to %qT
> > due to unsignedness of other operand"
> > 
> > or somesuch (am not 100% happy with that either).
> 
> Hmm, how about this, then?
> 
> "operand of ?: changes signedness from %qT to %qT due to unsignedness of 
> other operand"
> 
> I couldn't come up with anything more brief yet conveying all the information.
> I don't like adding "second"/"third"/... very much; we should offer a good
> location already.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2017-08-02  Marek Polacek  
> 
>   PR c/81417
>   * c-array-notation.c (fix_builtin_array_notation_fn): Update calls to
>   build_conditional_expr. 
>   * c-parser.c (c_parser_conditional_expression): Create locations for
>   EXP1 and EXP2 from their source ranges.  Pass the locations down to
>   build_conditional_expr.
>   * c-tree.h (build_conditional_expr): Update declaration.
>   * c-typeck.c (build_conditional_expr): Add location_t parameters.
>   For -Wsign-compare, also print the types.
> 
>   * input.c (make_location): New overload.
>   * input.h (make_location): Declare.
> 
>   * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call): Update
>   a call to build_conditional_expr.
> 
>   * Wsign-compare-1.c: New test.
>   * gcc.dg/compare1.c: Adjust dg-bogus.
>   * gcc.dg/compare2.c: Likewise.
>   * gcc.dg/compare3.c: Likewise.
>   * gcc.dg/compare7.c: Likewise.
>   * gcc.dg/compare8.c: Likewise.
>   * gcc.dg/compare9.c: Likewise.
>   * gcc.dg/pr11492.c: Likewise.
> 
> diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
> index e430f5c681b..40f1cfdabb8 100644
> --- gcc/c/c-array-notation.c
> +++ gcc/c/c-array-notation.c
> @@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
> *new_var)
>new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
> build_zero_cst (TREE_TYPE (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>new_var_init = build_modify_expr
> @@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
> *new_var)
>new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
> build_zero_cst (TREE_TYPE (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>new_var_init = build_modify_expr
> @@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
> *new_var)
>new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
> build_zero_cst (TREE_TYPE (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));   
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>new_var_init = build_modify_expr
> @@ -413,8 +416,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
> *new_var)
>new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), 

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

On Tue, Aug 01, 2017 at 04:48:01PM -0400, David Malcolm wrote:
> I'm wondering if the messages could use a slight rewording, to give a
> clue to the user about the reason *why* the expression has changed
> signedness.  The old message "signed and unsigned type in conditional
> expression" gave the clue (but failed to underline the subexpression
> changing sign, and tell what the old/new types were).
> 
> A horribly verbose way to put it would be something like:
> 
> "operand of conditional expression with mixed signedness changes
> signedness from %qT to %qT due to promotion to unsigned to match
> unsignedness of other operand" (ugh)
> 
> (assuming I'm understanding the logic correctly)
> 
> or something like:
> 
> "operand of conditional expression changes signedness from %qT to %qT
> due to unsignedness of other operand"
> 
> or somesuch (am not 100% happy with that either).

Hmm, how about this, then?

"operand of ?: changes signedness from %qT to %qT due to unsignedness of other 
operand"

I couldn't come up with anything more brief yet conveying all the information.
I don't like adding "second"/"third"/... very much; we should offer a good
location already.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-08-02  Marek Polacek  

PR c/81417
* c-array-notation.c (fix_builtin_array_notation_fn): Update calls to
build_conditional_expr. 
* c-parser.c (c_parser_conditional_expression): Create locations for
EXP1 and EXP2 from their source ranges.  Pass the locations down to
build_conditional_expr.
* c-tree.h (build_conditional_expr): Update declaration.
* c-typeck.c (build_conditional_expr): Add location_t parameters.
For -Wsign-compare, also print the types.

* input.c (make_location): New overload.
* input.h (make_location): Declare.

* objc-next-runtime-abi-02.c (build_v2_build_objc_method_call): Update
a call to build_conditional_expr.

* Wsign-compare-1.c: New test.
* gcc.dg/compare1.c: Adjust dg-bogus.
* gcc.dg/compare2.c: Likewise.
* gcc.dg/compare3.c: Likewise.
* gcc.dg/compare7.c: Likewise.
* gcc.dg/compare8.c: Likewise.
* gcc.dg/compare9.c: Likewise.
* gcc.dg/pr11492.c: Likewise.

diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
index e430f5c681b..40f1cfdabb8 100644
--- gcc/c/c-array-notation.c
+++ gcc/c/c-array-notation.c
@@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
   new_var_init = build_modify_expr
@@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
   new_var_init = build_modify_expr
@@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));   
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
   new_var_init = build_modify_expr
@@ -413,8 +416,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));   
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Martin Sebor]

I'm wondering if the messages could use a slight rewording, to give a
clue to the user about the reason *why* the expression has changed
signedness.  The old message "signed and unsigned type in conditional
expression" gave the clue (but failed to underline the subexpression
changing sign, and tell what the old/new types were).

A horribly verbose way to put it would be something like:

"operand of conditional expression with mixed signedness changes
signedness from %qT to %qT due to promotion to unsigned to match
unsignedness of other operand" (ugh)

(assuming I'm understanding the logic correctly)

or something like:

"operand of conditional expression changes signedness from %qT to %qT
due to unsignedness of other operand"

or somesuch (am not 100% happy with that either).


If I can make an observation I'd say (since you're not happy with
the wordiness) that mentioning signedness in addition to the types
of the operands is superfluous: it should be sufficiently clear
from the "from 'T' to 'unsigned T'" part.

That being said, what might be helpful is mentioning which operand's
type changes: the second or third.  That could be viewed as redundant
as well thanks to the underlining but only as long as the underlining
is correct and as long as it's not disabled by some option.

With that I'd suggest to consider the simple:

  second operand of conditional expression changes type from %qT to %qT

Martin

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[David Malcolm]

On Tue, 2017-08-01 at 16:15 +0200, Marek Polacek wrote:
> On Mon, Jul 31, 2017 at 11:31:44AM -0400, David Malcolm wrote:
> > On Mon, 2017-07-31 at 16:14 +0200, Marek Polacek wrote:
> > > This patch improves the diagnostic of -Wsign-compare for ?: by
> > > also
> > > printing
> > > the types, similarly to my recent patch.  But we can do even
> > > better
> > > here if we
> > > actually point to the operand in question, so I passed the
> > > locations
> > > of the
> > > operands from the parser.
> > 
> > Thanks for updating the patch.
> > 
> > >   So instead of 
> > > 
> > > x.c:8:16: warning: signed and unsigned type in conditional
> > > expression
> > > [-Wsign-compare]
> > >return x ? y : -1;
> > > ^
> > > you'll now see:
> > > 
> > > x.c:8:18: warning: operand of conditional expression changes
> > > signedness: 'int' to 'unsigned int' [-Wsign-compare]
> > >return x ? y : -1;
> > >   ^
> > 
> > That's an improvement, but I would have expected it to underline
> > the
> > whole of the pertinent subexpression e.g.:
> > 
> >return x ? y : -1;
> >   ^~
> > 
> > rather than just:
> > 
> >return x ? y : -1;
> >   ^
> > 
> > From my reading of the patch, it's capturing just the location of
> > the
> > first token within the subexpression (hence e.g. just the minus
> > token
> > in the example above, which happens to make some sense for this
> > case,
> > but wouldn't in general).
> 
> Right.
> 
> > Hopefully you can get at the location_t for the whole of the
> > subexpression using c_expr, rather than peeking at the first token.
> 
> It didn't seem obvious to me how to do that, but I've spent more time
> on this now.  I believe we need to use make_location (I've introduced
> a new overload), as in the patch below.  Or did you in mind anything
> else?

I think I was confused with the C++ frontend, where cp_expr has a
location_t representing all three of (caret, start, finish), whereas in
the C frontend c_expr has a source_range, representing just (start,
finish).

New overload looks reasonable.

> > > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> > 
> > The patch doesn't have a testcase for the location information;
> > please
> > add one, using -fdiagnostics-show-caret and dg-begin-multiline-
> > output/
> > dg-end-multiline-output.  Please ensure that the pertinent
> > expressions
> > are more than one character wide, so that the test properly
> > verifies
> > the underlining.
> 
> Done.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Thanks for updating the patch.

I'm wondering if the messages could use a slight rewording, to give a
clue to the user about the reason *why* the expression has changed
signedness.  The old message "signed and unsigned type in conditional
expression" gave the clue (but failed to underline the subexpression
changing sign, and tell what the old/new types were).

A horribly verbose way to put it would be something like:

"operand of conditional expression with mixed signedness changes
signedness from %qT to %qT due to promotion to unsigned to match
unsignedness of other operand" (ugh)

(assuming I'm understanding the logic correctly)

or something like:

"operand of conditional expression changes signedness from %qT to %qT
due to unsignedness of other operand"

or somesuch (am not 100% happy with that either).


Other than than, LGTM.


> 2017-08-01  Marek Polacek  
> 
>   PR c/81417
>   * c-array-notation.c (fix_builtin_array_notation_fn): Update
> calls to
>   build_conditional_expr. 
>   * c-parser.c (c_parser_conditional_expression): Create
> locations for
>   EXP1 and EXP2 from their source ranges.  Pass the locations
> down to
>   build_conditional_expr.
>   * c-tree.h (build_conditional_expr): Update declaration.
>   * c-typeck.c (build_conditional_expr): Add location_t
> parameters.
>   For -Wsign-compare, also print the types.
> 
>   * input.c (make_location): New overload.
>   * input.h (make_location): Declare.
> 
>   * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call):
> Update
>   a call to build_conditional_expr.
> 
>   * Wsign-compare-1.c: New test.
>   * gcc.dg/compare1.c: Adjust dg-bogus.
>   * gcc.dg/compare2.c: Likewise.
>   * gcc.dg/compare3.c: Likewise.
>   * gcc.dg/compare7.c: Likewise.
>   * gcc.dg/compare8.c: Likewise.
>   * gcc.dg/compare9.c: Likewise.
>   * gcc.dg/pr11492.c: Likewise.
> 
> diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
> index e430f5c681b..40f1cfdabb8 100644
> --- gcc/c/c-array-notation.c
> +++ gcc/c/c-array-notation.c
> @@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree
> an_builtin_fn, tree *new_var)
>new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm),
> func_parm,
> build_zero_cst (TREE_TYPE
> (func_parm)));
>new_expr = build_conditional_expr
> - 

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[David Malcolm]

On Mon, 2017-07-31 at 18:05 +0200, Marek Polacek wrote:
> On Mon, Jul 31, 2017 at 09:54:03AM -0600, Martin Sebor wrote:
> > On 07/31/2017 08:14 AM, Marek Polacek wrote:
> > > This patch improves the diagnostic of -Wsign-compare for ?: by
> > > also printing
> > > the types, similarly to my recent patch.  But we can do even
> > > better here if we
> > > actually point to the operand in question, so I passed the
> > > locations of the
> > > operands from the parser.  So instead of
> > > 
> > > x.c:8:16: warning: signed and unsigned type in conditional
> > > expression [-Wsign-compare]
> > >return x ? y : -1;
> > > ^
> > > you'll now see:
> > > 
> > > x.c:8:18: warning: operand of conditional expression changes
> > > signedness: 'int' to 'unsigned int' [-Wsign-compare]
> > >return x ? y : -1;
> > >   ^
> > 
> > I like that this is more informative than the last warning you
> > committed for this bug: it says what type the operand is converted
> > to.  The last one only shows what the types of the operands are but
> > leaves users guessing as to what that might mean (integer promotion
> > rules are often poorly understood).  Where the last warning prints
> > 
> >   comparison of integer expressions of different signedness: ‘int’
> > and
> > ‘unsigned int’
> > 
> > it would be nice to go back and add this detail to it as well, and
> > have it print something like this instead:
> > 
> >   comparison of integer expressions of different signedness changes
> > type of
> > the second operand from ‘int’ to ‘unsigned int’
> > 
> > Where constant expressions are involved it would also be helpful
> > to show the result of the conversion.  For instance:
> > 
> >   comparison between ‘int’ and ‘unsigned int’ changes the value of
> > the
> > second operand from ‘-1’ to ‘4294967296’
> 
> Hmm, interesting.  I could do that.  How do other people feel about
> this?
> 
>   Marek

I think that printing values is very helpful for convincing the user
that they need to listen to the compiler.

That said, we could do a better job of printing values (and boundary
values), especially for large values near a power of two; Martin filed
that as PR 80437; I've added some thoughts on that to that PR.

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

On Mon, Jul 31, 2017 at 11:31:44AM -0400, David Malcolm wrote:
> On Mon, 2017-07-31 at 16:14 +0200, Marek Polacek wrote:
> > This patch improves the diagnostic of -Wsign-compare for ?: by also
> > printing
> > the types, similarly to my recent patch.  But we can do even better
> > here if we
> > actually point to the operand in question, so I passed the locations
> > of the
> > operands from the parser.
> 
> Thanks for updating the patch.
> 
> >   So instead of 
> > 
> > x.c:8:16: warning: signed and unsigned type in conditional expression
> > [-Wsign-compare]
> >return x ? y : -1;
> > ^
> > you'll now see:
> > 
> > x.c:8:18: warning: operand of conditional expression changes
> > signedness: 'int' to 'unsigned int' [-Wsign-compare]
> >return x ? y : -1;
> >   ^
> 
> That's an improvement, but I would have expected it to underline the
> whole of the pertinent subexpression e.g.:
> 
>return x ? y : -1;
>   ^~
> 
> rather than just:
> 
>return x ? y : -1;
>   ^
> 
> From my reading of the patch, it's capturing just the location of the
> first token within the subexpression (hence e.g. just the minus token
> in the example above, which happens to make some sense for this case,
> but wouldn't in general).

Right.

> Hopefully you can get at the location_t for the whole of the
> subexpression using c_expr, rather than peeking at the first token.

It didn't seem obvious to me how to do that, but I've spent more time
on this now.  I believe we need to use make_location (I've introduced
a new overload), as in the patch below.  Or did you in mind anything
else?

> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> The patch doesn't have a testcase for the location information; please
> add one, using -fdiagnostics-show-caret and dg-begin-multiline-output/
> dg-end-multiline-output.  Please ensure that the pertinent expressions
> are more than one character wide, so that the test properly verifies
> the underlining.

Done.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-08-01  Marek Polacek  

PR c/81417
* c-array-notation.c (fix_builtin_array_notation_fn): Update calls to
build_conditional_expr. 
* c-parser.c (c_parser_conditional_expression): Create locations for
EXP1 and EXP2 from their source ranges.  Pass the locations down to
build_conditional_expr.
* c-tree.h (build_conditional_expr): Update declaration.
* c-typeck.c (build_conditional_expr): Add location_t parameters.
For -Wsign-compare, also print the types.

* input.c (make_location): New overload.
* input.h (make_location): Declare.

* objc-next-runtime-abi-02.c (build_v2_build_objc_method_call): Update
a call to build_conditional_expr.

* Wsign-compare-1.c: New test.
* gcc.dg/compare1.c: Adjust dg-bogus.
* gcc.dg/compare2.c: Likewise.
* gcc.dg/compare3.c: Likewise.
* gcc.dg/compare7.c: Likewise.
* gcc.dg/compare8.c: Likewise.
* gcc.dg/compare9.c: Likewise.
* gcc.dg/pr11492.c: Likewise.

diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
index e430f5c681b..40f1cfdabb8 100644
--- gcc/c/c-array-notation.c
+++ gcc/c/c-array-notation.c
@@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
   new_var_init = build_modify_expr
@@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
   new_var_init = build_modify_expr
@@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-   

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Martin Sebor]

On 07/31/2017 10:05 AM, Marek Polacek wrote:

On Mon, Jul 31, 2017 at 09:54:03AM -0600, Martin Sebor wrote:

On 07/31/2017 08:14 AM, Marek Polacek wrote:

This patch improves the diagnostic of -Wsign-compare for ?: by also printing
the types, similarly to my recent patch.  But we can do even better here if we
actually point to the operand in question, so I passed the locations of the
operands from the parser.  So instead of

x.c:8:16: warning: signed and unsigned type in conditional expression 
[-Wsign-compare]
   return x ? y : -1;
^
you'll now see:

x.c:8:18: warning: operand of conditional expression changes signedness: 'int' 
to 'unsigned int' [-Wsign-compare]
   return x ? y : -1;
  ^


I like that this is more informative than the last warning you
committed for this bug: it says what type the operand is converted
to.  The last one only shows what the types of the operands are but
leaves users guessing as to what that might mean (integer promotion
rules are often poorly understood).  Where the last warning prints

  comparison of integer expressions of different signedness: ‘int’ and
‘unsigned int’

it would be nice to go back and add this detail to it as well, and
have it print something like this instead:

  comparison of integer expressions of different signedness changes type of
the second operand from ‘int’ to ‘unsigned int’

Where constant expressions are involved it would also be helpful
to show the result of the conversion.  For instance:

  comparison between ‘int’ and ‘unsigned int’ changes the value of the
second operand from ‘-1’ to ‘4294967296’


Hmm, interesting.  I could do that.  How do other people feel about this?


Since you ask for input from others, let me mention that I also
would find it helpful if we could come up with some sort of high
level direction on what information to include in diagnostics and
how to present it (e.g., a section on the GCC Diagnostic Guidelines
page on the Wiki).  Not only would it help guide us in implementing
new diagnostics but it would also result in a more consistent and
uniform user experience.

For what it's worth, my own preference is to err on the side of
providing more detail rather than less so the changes I made in
r248431 to -Wconversion reflect that.  So for example there, GCC
might now print:

  unsigned conversion from ‘int‘ to ‘unsigned char‘ changes value from 
‘-128‘ to ‘128‘"


My suggestions above were based on the style I chose here.

Martin

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

On Mon, Jul 31, 2017 at 09:54:03AM -0600, Martin Sebor wrote:
> On 07/31/2017 08:14 AM, Marek Polacek wrote:
> > This patch improves the diagnostic of -Wsign-compare for ?: by also printing
> > the types, similarly to my recent patch.  But we can do even better here if 
> > we
> > actually point to the operand in question, so I passed the locations of the
> > operands from the parser.  So instead of
> > 
> > x.c:8:16: warning: signed and unsigned type in conditional expression 
> > [-Wsign-compare]
> >return x ? y : -1;
> > ^
> > you'll now see:
> > 
> > x.c:8:18: warning: operand of conditional expression changes signedness: 
> > 'int' to 'unsigned int' [-Wsign-compare]
> >return x ? y : -1;
> >   ^
> 
> I like that this is more informative than the last warning you
> committed for this bug: it says what type the operand is converted
> to.  The last one only shows what the types of the operands are but
> leaves users guessing as to what that might mean (integer promotion
> rules are often poorly understood).  Where the last warning prints
> 
>   comparison of integer expressions of different signedness: ‘int’ and
> ‘unsigned int’
> 
> it would be nice to go back and add this detail to it as well, and
> have it print something like this instead:
> 
>   comparison of integer expressions of different signedness changes type of
> the second operand from ‘int’ to ‘unsigned int’
> 
> Where constant expressions are involved it would also be helpful
> to show the result of the conversion.  For instance:
> 
>   comparison between ‘int’ and ‘unsigned int’ changes the value of the
> second operand from ‘-1’ to ‘4294967296’

Hmm, interesting.  I could do that.  How do other people feel about this?

Marek

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Martin Sebor]

On 07/31/2017 08:14 AM, Marek Polacek wrote:

This patch improves the diagnostic of -Wsign-compare for ?: by also printing
the types, similarly to my recent patch.  But we can do even better here if we
actually point to the operand in question, so I passed the locations of the
operands from the parser.  So instead of

x.c:8:16: warning: signed and unsigned type in conditional expression 
[-Wsign-compare]
   return x ? y : -1;
^
you'll now see:

x.c:8:18: warning: operand of conditional expression changes signedness: 'int' 
to 'unsigned int' [-Wsign-compare]
   return x ? y : -1;
  ^


I like that this is more informative than the last warning you
committed for this bug: it says what type the operand is converted
to.  The last one only shows what the types of the operands are but
leaves users guessing as to what that might mean (integer promotion
rules are often poorly understood).  Where the last warning prints

  comparison of integer expressions of different signedness: ‘int’ and 
‘unsigned int’


it would be nice to go back and add this detail to it as well, and
have it print something like this instead:

  comparison of integer expressions of different signedness changes 
type of the second operand from ‘int’ to ‘unsigned int’


Where constant expressions are involved it would also be helpful
to show the result of the conversion.  For instance:

  comparison between ‘int’ and ‘unsigned int’ changes the value of the 
second operand from ‘-1’ to ‘4294967296’


Martin



Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-07-31  Marek Polacek  

PR c/81417
* c-array-notation.c (fix_builtin_array_notation_fn): Update calls to
build_conditional_expr. 
* c-parser.c (c_parser_conditional_expression): Pass the locations of
OP1 and OP2 down to build_conditional_expr.
* c-tree.h (build_conditional_expr): Update declaration.
* c-typeck.c (build_conditional_expr): Add location_t parameters.
For -Wsign-compare, also print the types.

* objc-next-runtime-abi-02.c (build_v2_build_objc_method_call): Update
a call to build_conditional_expr.

* Wsign-compare-1.c: New test.
* gcc.dg/compare1.c: Adjust dg-bogus.
* gcc.dg/compare2.c: Likewise.
* gcc.dg/compare3.c: Likewise.
* gcc.dg/compare7.c: Likewise.
* gcc.dg/compare8.c: Likewise.
* gcc.dg/compare9.c: Likewise.
* gcc.dg/pr11492.c: Likewise.

diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
index e430f5c681b..40f1cfdabb8 100644
--- gcc/c/c-array-notation.c
+++ gcc/c/c-array-notation.c
@@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
   new_var_init = build_modify_expr
@@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
   new_var_init = build_modify_expr
@@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
   new_var_init = build_modify_expr
@@ -413,8 +416,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, 

Re: C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[David Malcolm]

On Mon, 2017-07-31 at 16:14 +0200, Marek Polacek wrote:
> This patch improves the diagnostic of -Wsign-compare for ?: by also
> printing
> the types, similarly to my recent patch.  But we can do even better
> here if we
> actually point to the operand in question, so I passed the locations
> of the
> operands from the parser.

Thanks for updating the patch.

>   So instead of 
> 
> x.c:8:16: warning: signed and unsigned type in conditional expression
> [-Wsign-compare]
>return x ? y : -1;
> ^
> you'll now see:
> 
> x.c:8:18: warning: operand of conditional expression changes
> signedness: 'int' to 'unsigned int' [-Wsign-compare]
>return x ? y : -1;
>   ^

That's an improvement, but I would have expected it to underline the
whole of the pertinent subexpression e.g.:

   return x ? y : -1;
  ^~

rather than just:

   return x ? y : -1;
  ^

>From my reading of the patch, it's capturing just the location of the
first token within the subexpression (hence e.g. just the minus token
in the example above, which happens to make some sense for this case,
but wouldn't in general).

Hopefully you can get at the location_t for the whole of the
subexpression using c_expr, rather than peeking at the first token.

> Bootstrapped/regtested on x86_64-linux, ok for trunk?

The patch doesn't have a testcase for the location information; please
add one, using -fdiagnostics-show-caret and dg-begin-multiline-output/
dg-end-multiline-output.  Please ensure that the pertinent expressions
are more than one character wide, so that the test properly verifies
the underlining.

Thanks
Dave


> 2017-07-31  Marek Polacek  
> 
>   PR c/81417
>   * c-array-notation.c (fix_builtin_array_notation_fn): Update
> calls to
>   build_conditional_expr. 
>   * c-parser.c (c_parser_conditional_expression): Pass the
> locations of
>   OP1 and OP2 down to build_conditional_expr.
>   * c-tree.h (build_conditional_expr): Update declaration.
>   * c-typeck.c (build_conditional_expr): Add location_t
> parameters.
>   For -Wsign-compare, also print the types.
> 
>   * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call):
> Update
>   a call to build_conditional_expr.
> 
>   * Wsign-compare-1.c: New test.
>   * gcc.dg/compare1.c: Adjust dg-bogus.
>   * gcc.dg/compare2.c: Likewise.
>   * gcc.dg/compare3.c: Likewise.
>   * gcc.dg/compare7.c: Likewise.
>   * gcc.dg/compare8.c: Likewise.
>   * gcc.dg/compare9.c: Likewise.
>   * gcc.dg/pr11492.c: Likewise.
> 
> diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
> index e430f5c681b..40f1cfdabb8 100644
> --- gcc/c/c-array-notation.c
> +++ gcc/c/c-array-notation.c
> @@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree
> an_builtin_fn, tree *new_var)
>new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm),
> func_parm,
> build_zero_cst (TREE_TYPE
> (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE
> (new_no_expr));
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
>new_var_init = build_modify_expr
> @@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree
> an_builtin_fn, tree *new_var)
>new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm),
> func_parm,
> build_zero_cst (TREE_TYPE
> (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE
> (new_no_expr));
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
>new_var_init = build_modify_expr
> @@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree
> an_builtin_fn, tree *new_var)
>new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm),
> func_parm,
> build_zero_cst (TREE_TYPE
> (func_parm)));
>new_expr = build_conditional_expr
> - (location, new_cond_expr, false, new_yes_expr,
> -  TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE
> (new_no_expr));   
> + (location, new_cond_expr, false,
> +  new_yes_expr, TREE_TYPE (new_yes_expr), location,
> +  new_no_expr, TREE_TYPE (new_no_expr), location);
>break;
>  case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
>new_var_init = build_modify_expr
> @@ -413,8 +416,9 @@ fix_builtin_array_notation_fn (tree
> an_builtin_fn, tree *new_var)
>new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm),
> 

C PATCH to further improve diagnostic for -Wsign-compare (PR c/81417)

[Marek Polacek]

This patch improves the diagnostic of -Wsign-compare for ?: by also printing
the types, similarly to my recent patch.  But we can do even better here if we
actually point to the operand in question, so I passed the locations of the
operands from the parser.  So instead of 

x.c:8:16: warning: signed and unsigned type in conditional expression 
[-Wsign-compare]
   return x ? y : -1;
^
you'll now see:

x.c:8:18: warning: operand of conditional expression changes signedness: 'int' 
to 'unsigned int' [-Wsign-compare]
   return x ? y : -1;
  ^

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-07-31  Marek Polacek  

PR c/81417
* c-array-notation.c (fix_builtin_array_notation_fn): Update calls to
build_conditional_expr. 
* c-parser.c (c_parser_conditional_expression): Pass the locations of
OP1 and OP2 down to build_conditional_expr.
* c-tree.h (build_conditional_expr): Update declaration.
* c-typeck.c (build_conditional_expr): Add location_t parameters.
For -Wsign-compare, also print the types.

* objc-next-runtime-abi-02.c (build_v2_build_objc_method_call): Update
a call to build_conditional_expr.

* Wsign-compare-1.c: New test.
* gcc.dg/compare1.c: Adjust dg-bogus.
* gcc.dg/compare2.c: Likewise.
* gcc.dg/compare3.c: Likewise.
* gcc.dg/compare7.c: Likewise.
* gcc.dg/compare8.c: Likewise.
* gcc.dg/compare9.c: Likewise.
* gcc.dg/pr11492.c: Likewise.

diff --git gcc/c/c-array-notation.c gcc/c/c-array-notation.c
index e430f5c681b..40f1cfdabb8 100644
--- gcc/c/c-array-notation.c
+++ gcc/c/c-array-notation.c
@@ -355,8 +355,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO:
   new_var_init = build_modify_expr
@@ -375,8 +376,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO:
   new_var_init = build_modify_expr
@@ -394,8 +396,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (EQ_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));   
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO:
   new_var_init = build_modify_expr
@@ -413,8 +416,9 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_cond_expr = build2 (NE_EXPR, TREE_TYPE (func_parm), func_parm,
  build_zero_cst (TREE_TYPE (func_parm)));
   new_expr = build_conditional_expr
-   (location, new_cond_expr, false, new_yes_expr,
-TREE_TYPE (new_yes_expr), new_no_expr, TREE_TYPE (new_no_expr));   
+   (location, new_cond_expr, false,
+new_yes_expr, TREE_TYPE (new_yes_expr), location,
+new_no_expr, TREE_TYPE (new_no_expr), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX:
   if (TYPE_MIN_VALUE (new_var_type))
@@ -434,7 +438,8 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)
   new_expr = build_conditional_expr
(location,
 build2 (LT_EXPR, TREE_TYPE (*new_var), *new_var, func_parm), false,
-new_yes_expr, TREE_TYPE (*new_var), new_no_expr, TREE_TYPE (*new_var));
+new_yes_expr, TREE_TYPE (*new_var), location,
+new_no_expr, TREE_TYPE (*new_var), location);
   break;
 case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN:
   if (TYPE_MAX_VALUE (new_var_type))
@@ -454,7 +459,8 @@ fix_builtin_array_notation_fn (tree an_builtin_fn, tree 
*new_var)