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

[Jesper Juhl]

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

-
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/