[cfe-users] -Wconversion does not produce warnings when it should

[David Blaikie via cfe-users]

On Mon, Nov 16, 2020 at 12:36 PM Sven Köhler via cfe-users
<cfe-users at lists.llvm.org> wrote:
>
> Hi,
>
> consider the following code:
>
> uint16_t testa4(uint8_t x, uint8_t y) {
>     return ( (uint16_t)x << 0 )
>             | ( (uint16_t)y << 8 );
> }
> uint16_t testb3(uint16_t x, uint16_t y) {
>     return x|y;
> }
> uint16_t testb4(uint16_t x, uint16_t y) {
>     return x+y;
> }
> int testb4x(uint16_t x, uint16_t y) {
>     return x+y;
> }
>
>
>
> I'm using clang 11.0.0 on Intel x86_64 and when I compile the above code
> with -Wconversion enabled, then I see only one warning, namely for testa4.
>
> However, according to the C standard, the expression after the return
> statement has type int in all 4 cases. So why is the warning not
> triggered for testb3 and testb4?
>
> In fact, it's convenient that testb3 does not trigger a warning. Yes, x
> and y are promoted to int before the bitwise or is performed, but the
> result indeed fits into an uint16_t, regardless of x and y. So if clang
> is smart and keeps track of the range of the expressions, then it is
> understandable that there is no warning for testb3. However, then the
> "keeping track of ranges" feature seems to fail for testa4, because
> there a warning is indeed produced even though the two operands of the
> bitwise-or are both uint16_t.
>
> However, the case of testb4 is different. If we set x an y to 0xFFFF,
> then the result will be 0x1FFFE. You can confirm that this is the case
> by running testb4x. Then, shouldn't testb4 trigger a warning? The result
> is clearly and int _and_ the result may not fit into an uint16_t.
>
>
> I'm lost. As far as I understand, -Wconversion should issue a warning
> when an int is converted to uint16_t without an explicit cast. For all I
> know, x+y and x|y are ints, yet there is no warning in testb3 and
> testb4. This seems like a bug to me.
>
>
> Can you elaborate what is happening here?

I believe/would guess the goal is to reduce false positives (where a
false positive is defined as "warning on code where the code does what
the author intended").

In the taste of testb4 this code is "as good as" the unsigned int
equivalent, say (unigned x(unsigned a, unsigned b) { return a + b; })
- wraparound occurs in both cases, so the code perhaps isn't too
surprising/the semantics of widening/narrowing conversions don't
really impact the behavior. The uint16 version of the code and the
unsigned int version of the code would appear to the user to work in
the same way.

But, yeah, once the expressions are sufficiently complicated, as in
testa4, clang's attempt to silence benign cases of the warning gives
up and you get warnings. It's best-effort sort of stuff.