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

[Jesper Juhl]

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@xxxxxx>
> >
> > 	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



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/