Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

Jesper Juhl
2004-11-23T18:41:45Z

On Tue, 23 Nov 2004, Linus Torvalds wrote:
>
>
> On Tue, 23 Nov 2004, Jesper Juhl wrote:
> >
> > Linus, would you accept patches like this?
>
> No, please don't.
>
> The warning is sometimes useful, but when it comes to a construct like
>
> if (a < 0 || a > X)
>
> the fact that "a" is unsigned does not make the construct silly. First
> off, it's (a) very readable and (b) the type of "a" may not be immediately
> obvious if it's a user typedef, for example.
>
> In fact, the type of "a" might depend on the architecture, or even
> compiler flags. Think about "char" - which may or may not be signed
> depending on ABI and things like -funsigned-char.
>
> In other places, it's not "unsigned" that is the problem, but the fact
> that the range of a type is smaller on one architecture than another. So
> you might have
>
> inf fn(pid_t a)
> {
> if (a > 0xffff)
> ...
> }
>
> which might warn on an architecture where "pid_t" is just sixteen bits
> wide. Does that make the code wrong? Hell no.
>
I'm aware that there are pitfalls, one of the very first things I looked
at was the usage of FIRST_USER_PGD_NR in mm/mmap.c:1513 On my main
platform (i386) FIRST_USER_PGD_NR is zero which causes gcc -W to warn
about if (start_index < FIRST_USER_PGD_NR) but, after seeing that it
is not 0 on all platforms I left that one alone.

> IOW, a lot of the gcc warnings are just not valid, and trying to shut gcc
> up about them can break (and _has_ broken) code that was correct before.
>
Shutting up gcc is not the primary goal here, the goal is/was to
a) review the code and make sure that it is safe and correct, and fix it
when it is not.
b) remove comparisons that are just a waste of CPU cycles when the result
is always true or false (in *all* cases on *all* archs).

> > I probably won't be able to properly evaluate/review *all* the instances
> > of this in the kernel,
>
> It's not even that I will drop the patches, it's literally that "fixing"
> the code so that gcc doesn't complain can be a BUG. We've gone through
> that.
>
I'll keep that firmly in mind and only submit patches for these kind of
things if I find usage that is actually (provably) buggy or where it's
completely clear that a comparison will *always* be true or false on all
architectures and removing it does not decrease readability.
I hope that's a more resonable aproach.

Whether or not the list of potential patches is now down to zero remains
to be seen, but it just got a hell of a lot shorter. :)

Thank you for the feedback.

--
Jesper Juhl