Messages in this thread | | | Date | Tue, 27 Jun 2006 10:23:29 +0200 | Subject | Re: [PATCH] Clean up the bootmem allocator | From | Franck Bui-Huu <> |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |