Re: [boost] Current Guidance on Compiler Warnings?

Subject: Re: [boost] Current Guidance on Compiler Warnings?
From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2018-11-27 08:59:17

Am 26.11.18 um 21:40 schrieb Emil Dotchevski via Boost:
>>> 1) A cast has different semantics than an implicit conversion.
>>>
>>> 2) If the implicit conversion is correct, replacing it with a cast to
>>> silence a warning is incorrect.
> The first is a fact, the second is basic logic. Both are true.
Consider: int x=1; unsigned y=2; if(static_cast<unsigned>(x) < y) ...
Where does this cast change any semantics? Implicit conversion changes x 
to unsigned *in this case*, the cast explicitly does the same and hence 
has the same semantics and is correct too which contradicts both points.
Of course this may no longer be true if code is refactored but for 
refactoring you always need to check what you do.
> (The cast converts to the type you specify in the cast. The implicit
> conversion converts to the target type.)
So? You can even use both: short  x=1; unsigned y=2; 
if(static_cast<unsigned short>(x) < y)
> What you're saying about disabling the warning is equally true about the
> cast. You're thinking that if you look at it and say it's ok, then the cast
> can't be wrong, but that's false. If you change the compiler or the
> platform, or under maintenance, the cast may become the cause of a bug,
> which it will then hide because you've told the compiler you know what
> you're doing.
The only valid argument is "under maintenance", the others are wrong: If 
you locally checked that the implicit cast is correct (e.g. by asserts, 
logic, ...) and make it explicit it will be correct too UNLESS types are 
changed afterwards without checking the cast. The same applies to the 
implicit cast!
> unsigned f();
>
> void g( int x )
> {
>    if( static_cast<unsigned>(x) < f() ) //x is guaranteed to be >=0
>      ....
> }
>
> Elsewhere:
>
> g(-1);

That is the example I have brought: Either you can control ALL calls of 
g because it is local enough so you can ensure its argument is 
non-negative the first version is correct. Otherwise the following is 
better:

void g( int x )
{
   assert(x>=0);
   if( static_cast<unsigned>(x) < f() )
     ....
}

The cast will tell the compiler, that this is correct. So the compiler 
will not alert everyone that it may not be correct. Without the cast and 
assert the code will be wrong exactly for the same example you brought: 
`g(-1)` and as there are 100s of warnings of that kind you ignore them 
so you don't find this one case where it has been correct...

> First, the assert will catch the bug, while the cast will hide it.
> Secondly, the implicit conversion is both correct and more likely to remain
> correct under maintenance, because the compiler will pick the best
> conversion if the types change.
It won't pick the "best" conversion. Why is for "int < unsigned" to 
conversion to unsigned the "best"?
> I'm not against doing something that suppresses the warning without
> changing the program semantically, though these things tend to be ugly and
> difficult to port, so they're impractical.
How is the assert-and-cast approach ugly and difficult to port?
> But adding a cast changes the behavior of the program. It is not a good
> idea to break a correct program in order to suppress a bogus warning.

If you are so concerned use own functions: `auto unsigned_cast(T v){ 
assert(v>=0); static_cast<make_unsigned_t<T>>(v); }

Simple, safe, expressive and portable. Of course it breaks if you change 
the comparand to be both signed which now reintroduces the problem but 
produces a warning again so you easily find it.

 > signed integer overflow has undefined behavior. unsigned integers 
wrap. This means that signed integers give the compiler more scope for 
optimization and/or runtime checks.

This optimization scope is quite narrow and now you have possibly 
introduced UB into your program.

> It's not perfectly fine, because you can pass a negative index to it and there's no way to check for that (from within the function). If you take a signed type, you can assert.

MSVC warns when passing a signed to an unsigned param: 
https://godbolt.org/z/1ydXuL So you would need to assert-and-cast at the 
call site but you don't need such asserts for e.g. `for(int i=0; i<10; 
i++) a.set(i) = i;`

> I don't see your point though, are you saying that implicit conversions are dangerous and we should avoid them, using casts every time an implicit conversion might occur?

Only for signed<>unsigned conversions. Widening implicit conversions are 
fine as they are safe.

>void f( unsigned );
>
>void g( int x )
>{
>  f(x);
>}
>I don't think you'll get a warning. But I might be wrong, so let's say you
>do get a warning, so you do:
>void g( int x )
>{
>  f( static_cast<unsigned>(x) );
>}

MSVC does: https://godbolt.org/z/1ydXuL
And the 2nd one is missing reasoning why this cast is correct (whether 
explicit or implicit) and potentially an assert. With the given 
information the original code is wrong already.