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-26 14:21:03

Am 26.11.18 um 14:50 schrieb degski:
> On Mon, 26 Nov 2018 at 10:12, Alexander Grund via Boost 
> <boost_at_[hidden] <mailto:boost_at_[hidden]>> wrote:
>
>     IF you know absolutely sure that this signed value is always
>     non-negative, then why not:
>     a) Change the parameter type to unsigned to communicate this as a
>     precondition of the function
>
>     b) add a cast to the unsigned parameter type and potentially an
>     assert
>     if this helps future readers and catch precondition violations
>
>     I don't see where this cast is wrong. It does the same as the
>     implicit
>     cast but explicit and hence conveys *intend*.
>
> But it [any of those options] will never alert you to a violation of 
> that precondition. Making std::size_t unsigned is a mistake IMO.

- In option a) you get a compiler warning in the calling code when the 
precondition is violated (conversion from signed to unsigned)

- In option b) the assert will tell you that

I might agree with the size_t mistake: Using it for sizes may be ok, but 
as an index type might have been wrong.

> If you do as Emil proposes, run a debug build with [that type of] 
> asserts you'll **know** that **your** surrounding code [or **your** 
> input ] is wrong and you'll **know** something needs fixing (not with 
> a cast, but **your** [surrounding] logic). To me his [Emil's] argument 
> is convincing, casting just hides the problem

Correct: The surrounding code needs fixing, not the code with the 
now-added cast. The warning about the "missing" cast can never point you 
to the surrounding code. It is a violation of the contract of the 
function to be called with a negative value. You can catch this with an 
assert or move the responsibility up to the caller (change param type)

There is another point about this: This function might be an internal 
one where you have full control over, maybe even a lambda. There *may* 
be cases where you can guarantee that this is not called with negative 
values (the local lambda, an internal class function only locally 
accessible, ...) but normally you either assert-and-cast or change the 
param type leading to e.g.:
for(int i=0; i<42;i++) foo(static_cast<unsigned>(i));

The cast can be easily validated locally, no assert required.

> coz you just shot yourself in the foot and you don't know where the 
> gun is (carrying a concealed weapon is a criminal offense in the US 
> AFAIK ;-) )].
If you do not suppress the warnings (after careful investigation) how do 
you find real issues highlighted by warnings? Using your metaphor: How 
do you find the serial killer with the gun if everyone has a gun and 
shoots wildly around?
> We should have a dynamic_assert b.t.w., i.e. the converse of 
> static_assert (in C++, not the C macro).

I prefer something like this: template<class T, class U> 
enable_if_t<is_unsigned<T> && is_signed<U>, T> checked_cast(U val){ 
assert(val >= 0); return static_cast<T>(val); }

Similar for conditions for narrowing casts or unsigned->signed casts.

If appropriate use an own define for assert that may stay enabled in 
release builds. Note that for my loop example above the check will be 
optimized out by teh compiler as it knows that i>=0