Re: CVS commit: src/external/bsd/dhcpcd/dist/src

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Thu, Feb 13, 2020 at 18:25:41 +0100, Kamil Rytarowski wrote:

> On 13.02.2020 18:00, Valery Ushakov wrote:
> > Arguably, if the tool you use is broken, you shouln't be mutilating
> > the source code just to shut that tool up. 
> 
> The introduced changes were good, even if GCC would be silent.

You changed one occurence just because it happens to trigger a bug in
gcc.  There are ntohs() >< size_t_var comparisons right above and
below the line you changed, where the same promotion happens (without
triggering a gcc bug) and they don't have a cast.  So someone reading
that code will ask themself, wait a minute, why the cast is necessary
in *that* place but not in those places?  What is going on there that
I miss that required introduction of that cast.  I.e. your change
create cognitive load for the reader.  I don't cosider that good.


> [...] the promotion rules are considered by many people as the major
> flaw in C.  Today I prefer explicit casts (after the MUSL lesson)
> even if unnecessary than implicit promotion.

Amen.  However they are there and we made uneasy peace with them so
unless you are consistent in your casting, you send all the wrong
singals to the reader.


> As an alternative option we can disable warnings but this is in my
> opinion much worse in this case than potentially overlooking real
> problems in a 4000+ line file.

I did not propose to disable the warning.  I proposed to downgrade
-Werror to -Wno-error (i.e. a warning) and only for the buggy
sanitizer build.  That file will still be compiled in normal builds
with all the warnings=errors enabled, so real problems won't be
overlooked.


> Repeatedly informing that the tool (GCC) is broken did not solve any
> issue.  It would be finally better to see someone fixing GCC rather
> than informing other GCC users about it.

Feel free to follow your own advice here...  (After all, it's your
sanitizer usage that has problems.  The normal builds are fine as they
are :)


> On the opposite side of this if this camp is MUSL. I used chunks of the
> MUSL code and it had poor results. There is a strict policy to avoid
> casts unless absolutely needed

That's not a bad policy.  As I said, a cast is a blunt tool.  It's
easy to introduce a wrong one that would silence some warning or other
but will do the wrong thing, but now since there is a cast the
compiler cannot even squeak.  I might be misremembering, but IIRC
Visual C has some warning(s) that ignore explicit casts, precisely to
detect that kind of problems.


> and if they are needed this tends to be in as obscure way as
> possible (like adding U attribute to one of the arguments in an
> expression).

What's obscure about adding 'U' to signify unsignedness?!  Also it's
not a cast to begin with, a literal with the 'u' suffix *is* unsigned.
A cast is when you take something of one type and coerce it to be of
some other type.


-uwe