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

Jesper Juhl
2004-11-23T09:46:26Z

Matthew Wilcox wrote:
> On Sun, Nov 21, 2004 at 11:55:23PM +0100, Jesper Juhl wrote:
>
>>This patch removes a pointless comparison. "arg" is an unsigned long, thus
>>it can never be <0, so testing that is pointless.
>>
>>Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
>>
>> case F_SETSIG:
>> /* arg == 0 restores default behaviour. */
>>- if (arg < 0 || arg > _NSIG) {
>>+ if (arg > _NSIG) {
>> break;
>
>
> I've seen patches like this before. I'm generally in favour of removing
> the unnecessary test, but Linus rejected it on the grounds the compiler
> shouldn't be warning about it and it's better to be more explicit about
> the range test. Maybe he's changed his mind between then and now ;-)
>
Let's find out.
Linus, would you accept patches like this?

I've been building recent kernels with -W and there are tons of places
with similar comparisons like the one above, as well as places where
signed and unsigned values are compared, places where values are
potentially truncated in signed/unsigned assignments and similar.
At the very least a review of these code locations to make sure they are
all safe would make sense, and I think it would also make sense to get
rid of the comparisons that always evaluate true or false due to the
signedness or range of datatypes.
I probably won't be able to properly evaluate/review *all* the instances
of this in the kernel, but I don't mind spending some time reviewing
what I can and submit patches, but I don't want to waste my time with it
if you already know up-front that you'll just be dropping all such
patches. Or is this up to the individual maintainers to accept/reject?
If so, then I'll go ahead and submit patches, then the individual
maintainers can ack/nack as they please.

Comments?

--
Jesper Juhl