Re: [PATCH] Fix -Wconversion (PR c++/34198)

["Manuel López-Ibáñez" <lopezibanez at gmail dot com>]

On 23/11/2007, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The new -Wconversion and -Wsign-conversion implementation in 4.3 warns even
> when the C resp. C++ FE decides to shorten some binary operation (e.g.
> bitwise ops) or even for say
> unsigned char a;
> ...
> unsigned char b = (int) a;
> where the implicit narrowing conversion just undoes previous widening
> conversion.  The following patch fixes that by looking through any NOP_EXPRs
> in EXPR seeing if the warning would be pointless.
>
> I have tried to use get_unwidened as well, but that results in some warnings
> which are desirable to be omitted.  Say for:
> signed char sc;
> ...
> signed char i = (int) (unsigned short int) sc;
> when using expr = get_unwidened (expr, expr) instead of get_narrower
> it doesn't warn, eventhough the implicit conversion from (int) (ushort) sc
> to (signed char) really can change the value.  Consider sc = -1, here
> (int) (unsigned short int) sc
> is 0xffff, while (signed char) 0xffff is -1.  get_unwidened does that,
> because the outermost widening conversion is skipped over and then
> there are just conversions with equal precision.  With get_narrower
> it does the right thing.

Shouldn't the call be expr = get_unwidened (expr, totype) ? The
comments about get_unwidened precisely talk about this.

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

I am sorry, but this is not OK. Explicit casts should avoid warning
always. This issue doesn't need an explicit cast to produce the
incorrect warning:

void f(unsigned char b)
{
  if (b & 0xff) {
    return;
  }
}

we get:

test.C:3: warning: conversion of '(int)b' to 'unsigned char' from
'int' may alter its value

This is wrong. There is no conversion from int to unsigned char. The
only conversion is from unsigned char promoted to int and that is
correctly not warned.

In fact for:

void g(unsigned char b)
{
 unsigned short c = b & 0xff;
}

we currently get:

wconversion-3.c:3: warning: conversion to 'unsigned char' from 'int'
may alter its value
wconversion-3.c:3: warning: conversion to 'short unsigned int' from
'int' may alter its value

The second warning is probably warranted. That is we cannot know that
the conversion of int(b & 0xff) to unsigned short is safe unless we
look into the operands. And we don't do that for any other operation.
Moreover, a user cast would suppress the warning.

However, the first warning is a bug. There shouldn't be any conversion
from int to unsigned char, we are introducing it artificially and
implicitly in such a way that no user cast can avoid it. Actually, if
I read the code correctly in c-typeck.c's build_bin_op, the conversion
is only introduced if it is safe, since this is some kind of
optimization.

      /* For certain operations (which identify themselves by shorten != 0)
	 if both args were extended from the same smaller type,
	 do the arithmetic in that type and then extend.

	 shorten !=0 and !=1 indicates a bitwise operation.
	 For them, this optimization is safe only if
	 both args are zero-extended or both are sign-extended.
	 Otherwise, we might change the result.
	 Eg, (short)-1 | (unsigned short)-1 is (int)-1
	 but calculated in (unsigned short) it would be (unsigned short)-1.  */

Let me know what you think about all this.

Cheers,

Manuel.