On Mon, Jul 21, 2014 at 2:08 AM, Anton Lundin glance@acc.umu.se wrote:
This fixes a always false comparison that a unsigned int can never be below zero. This was found when compiling with clang.
.. and I think clang is pure and utter shit in that case.
The thing is, just look at your patch:
@@ -142,7 +142,7 @@ dc_ihex_file_read (dc_ihex_file_t *file, dc_ihex_entry_t *entry)
/* Get the record type. */ type = data[3];
if (type < 0 || type > 5) {
if (type > 5) { ERROR (file->context, "Invalid record type (0x%02x).", type); return DC_STATUS_DATAFORMAT; }
and ask yourself: "Which version is better? Before or after the patch?"
The original code is better and clearer. It clearly does a range comparison, and it clearly verifies that "type" is between 0-5. Agreed?
The code *after* the patch is unsafe and unclear. What happens if somebody changes the type of "type" for some reason? And even if nobody does that, the actual type definition is *easily* off the screen unless you have a big screen, so for a *human*, the "if (type > 5)" test is actually inherently weaker, because the human - unlike a compiler - doesn't always look at the types.
In other words, libdivecomputer source gets this right, and clang is a bag of putrid crap for warning about the better code. The warning makes no sense and is pure and utter shit. Your patch should be to make clang shut up, not to make libdivecomputer source code worse.
clang should instead say to itself "good for you, you wrote nice and understandable code, and I can just optimize the code to a single unsigned comparison, so there is no downside to your safe code. YAY!".
Now, if you have code like "type < -1" then *that* kind of comparison is worth warning about, but clang is just utterly wrong to warn about sane and safe range comparisons like this.
Linus