Boost logo

Boost :

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.




Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk