[PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Jesper Juhl]

In mm/slab.c in the function kmem_cache_create, there's a check for
'offset < 0' that is completely unnessesary since 'offset' is of
type size_t, and size_t is an unsigned datatype in all archs.

The patch below removes this un-needed code - even gcc agrees that this
code is not needed and that case can never happen.


--- linux-2.6.1-rc1-mm2-orig/mm/slab.c  2004-01-06 01:33:09.000000000 +0100
+++ linux-2.6.1-rc1-mm2/mm/slab.c       2004-01-08 02:08:33.000000000 +0100
@@ -1042,7 +1042,7 @@ kmem_cache_create (const char *name, siz
 		(size < BYTES_PER_WORD) ||
 		(size > (1<<MAX_OBJ_ORDER)*PAGE_SIZE) ||
 		(dtor && !ctor) ||
-		(offset < 0 || offset > size))
+		(offset > size))
			BUG();

 #if DEBUG


Patch is compile tested, that's all - but it seems to be 'obviously
correct' to me.


Kind regards,

Jesper Juhl



PS. CC'ing the people mentioned in the comments of mm/slab.c since I
couldn't fine any single person responsible for this file in MAINTAINERS -
hope that's OK.

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Joe Perches]

On Wed, 2004-01-07 at 17:20, Jesper Juhl wrote:
> size_t is an unsigned datatype in all archs.

There are literally hundreds of these in the code.

Compile with -W -Wall and see for yourself.

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Jesper Juhl]

On Wed, 7 Jan 2004, Joe Perches wrote:

> On Wed, 2004-01-07 at 17:20, Jesper Juhl wrote:
> > size_t is an unsigned datatype in all archs.
>
> There are literally hundreds of these in the code.
>
> Compile with -W -Wall and see for yourself.
>

Well, anything wrong in cleaning them up?


-- Jesper Juhl

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Paul Jackson]

Jason asked:
> Well, anything wrong in cleaning them [unsigned compare warnings] up?

It's more important that we write code that will fit in our limited
human brains than that we write code that will avoid spurious warnings
from gcc ('spurious' meaning warnings for code that gcc will correctly
compile anyway).

Or, see a couple months ago, in a thread with the Subject of:

  [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len

in which Linus wrote:
> That's why I hate the "sign compare" warning of gcc so much - it warns 
> about things that you CANNOT sanely write in any other way. That makes 
> that particular warning _evil_, since it encourages people to write crap 
> code.


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Jens Axboe]

On Thu, Jan 08 2004, Paul Jackson wrote:
> Jason asked:
> > Well, anything wrong in cleaning them [unsigned compare warnings] up?
> 
> It's more important that we write code that will fit in our limited
> human brains than that we write code that will avoid spurious warnings
> from gcc ('spurious' meaning warnings for code that gcc will correctly
> compile anyway).
> 
> Or, see a couple months ago, in a thread with the Subject of:
> 
>   [PATCH] irda: fix type of struct irda_ias_set.attribute.irda_attrib_string.len
> 
> in which Linus wrote:
> > That's why I hate the "sign compare" warning of gcc so much - it warns 
> > about things that you CANNOT sanely write in any other way. That makes 
> > that particular warning _evil_, since it encourages people to write crap 
> > code.

That's fine and has its place, no doubt about that. It doesn't cover the
patch in this thread though. The check is dead code. It's a cosmetic
problem though, gcc should not generate the code checking for < 0.

-- 
Jens Axboe

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Joe Perches]

On Thu, 2004-01-08 at 02:16, Paul Jackson wrote:
> Jason asked:
> > Well, anything wrong in cleaning them [unsigned compare warnings] up?

In this case the warning is not unsigned compare but
"comparison of .* is always [true|false]".

This sort of code generally makes me think someone did something wrong,
not just that the person added additional unnecessary checking.

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Manfred Spraul]

Jens Axboe wrote:

>That's fine and has its place, no doubt about that. It doesn't cover the
>patch in this thread though. The check is dead code. It's a cosmetic
>problem though, gcc should not generate the code checking for < 0.
>  
>
I'll fix it when I touch that area again. Probably with the patch that 
allows constructors to fail, which would be 2.7 stuff. It's not really 
urgent.

--
    Manfred

Re: [PATCH] mm/slab.c remove impossible <0 check - size_t is not signed - patch is against 2.6.1-rc1-mm2

[Bill Davidsen]

Joe Perches wrote:
> On Thu, 2004-01-08 at 02:16, Paul Jackson wrote:
> 
>>Jason asked:
>>
>>>Well, anything wrong in cleaning them [unsigned compare warnings] up?
> 
> 
> In this case the warning is not unsigned compare but
> "comparison of .* is always [true|false]".
> 
> This sort of code generally makes me think someone did something wrong,
> not just that the person added additional unnecessary checking.

Agreed, often muddy thinking.


-- 
bill davidsen <davidsen@tmr.com>
   CTO TMR Associates, Inc
   Doing interesting things with small computers since 1979