Re: [PATCH] Clean up the bootmem allocator

[Franck Bui-Huu <vagabon.xyz@gmail.com>]

Dave Hansen wrote:
> On Mon, 2006-06-26 at 15:11 +0200, Franck Bui-Huu wrote:
>> This patch does _only_ some cleanup in the bootmem allocator by:
> ...
>>  include/linux/bootmem.h |  101 ++++++++++++++----------
>>  mm/bootmem.c            |  195 ++++++++++++++++++++++++++---------------------
>>  2 files changed, 167 insertions(+), 129 deletions(-)
> 
> I'm always suspicious of "cleanups" which add more code than they remove ;)
> 

it's mainly due to the "80 columns limit" changes

>>         - following the kernel coding style conventions
> 
> Above everything else, this probably needs to be in its very own patch,
> where it is trivially verifiable. 
> 
>>         - using pfn/page conversion macros
>>         - removing some not needed parentheses
>>         - removing some useless included headers
>>         - limiting to 80 column width
> 
> In general, I think there is some good stuff in here.  However, instead
> of concentrating on "kernel coding style conventions" and numeric (80
> column) guidelines, I really hope that people consider _readability_
> when modifying this code.  I don't think this patch makes the code much
> more readable.
> 

well, IMHO using pfn/page conversion macros makes the code more readable.

> That said, there are some nice helper function in here.  Would you be
> able to break this patch up into maybe 10 or 15 smaller patches?  I have
> the feeling it will be easier to find the good bits then.
> 

ok I'll split that.

>> It also removes __init tags in the header file and hopefully make it
>> easier to read. 
> 
> I think I kinda like when these are present in headers.  I usually
> stumble across the header declarations before I do the ones in the .c
> files, and it is always nice to see the header visually _matching_
> the .c file, and how the variable is intended to be used
> 

see Andrew's comments...

		Franck