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
