[PATCH] Unsigned int type can't be below zero

[Linus Torvalds]

On Mon, Jul 21, 2014 at 2:08 AM, Anton Lundin <glance at 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