Re: [boost] Current Guidance on Compiler Warnings?

Subject: Re: [boost] Current Guidance on Compiler Warnings?
From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2018-11-26 20:40:28

On Mon, Nov 26, 2018 at 12:12 AM Alexander Grund via Boost <
boost_at_[hidden]> wrote:
>
>
> Am 25.11.18 um 04:31 schrieb Emil Dotchevski via Boost:
> > Here is my argument again:
> >
> > 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.
> >
> > Which of these two points do you think is false?
>
> I'd argue both are false because they are to general.  If you add "may"
> then these arguments "may" be true.

The first is a fact, the second is basic logic. Both are true.

> I'll include your further reply:
>
> > The reason is that they have the wrong mindset when approaching the
problem.
> >
> > They think: I get a warning, because the compiler doesn't know that this
> > signed value is always positive, but I know that it is, so I'm going to
> > cast, to tell the compiler that I know what I'm doing.
> >
> > A better reasoning is: I'm converting a signed value to unsigned value,
> > which will not be correct if the signed value is negative. With this in
> > mind, you'll notice that the cast is utterly useless in catching the
actual
> > error the compiler is warning you about.
>
> I can't really follow you here: The compiler not knowing that the signed
> value (which per definition CAN in general be negative) is always
> positive and alerting you of this is the whole point.
> 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

This is not what unsigned is for. You should generally use signed ints even
if the value can't be negative.

> I don't see where this cast is wrong. It does the same as the implicit
> cast but explicit and hence conveys *intend*.

Fact: the cast does not do the same thing as the implicit conversion.

(The cast converts to the type you specify in the cast. The implicit
conversion converts to the target type.)

> Disabling this warning might hide instances where such implicit
> conversions are in fact wrong.

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.

> I also see a contradiction in your argument: "this signed value is
> always positive, [...] I know that it is" is a valid reason to
> "converting a signed value to unsigned value, which will not be correct
> if the signed value is negative". So why is "the cast [...] utterly
> useless in catching the actual error"? If you know (or can ensure) the
> precondition then there is no error and you putting in the cast states
> that you thought about it.

unsigned f();

void g( int x )
{
  if( static_cast<unsigned>(x) < f() ) //x is guaranteed to be >=0
    ....
}

Elsewhere:

g(-1);

Would the cast or the comment catch this bug? No. Compare with:

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

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.

And if you worry about truncation under maintenance, same reasoning
applies: add asserts rather than comments or casts.

> In conclusion: These warnings are about suspicious code that *needs* to
> be inspected. After inspection relevant action must be taken which
> should result in the warning being solved or suppressed so the next
> reviewer does not need to inspect it again unless surrounding code has
> changed.

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.

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.