| Message ID | 20230805175027.50029-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lib/vsprintf: Rework header inclusions | expand |
On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote: > Sorting headers alphabetically helps locating duplicates, and > make it easier to figure out where to insert new headers. I agree that includes become a mess after some time. But I am not persuaded that sorting them alphabetically in random source files help anything. Is this part of some grand plan for the entire kernel, please? Is this outcome from some particular discussion? Will this become a well know rule checked by checkpatch.pl? I am personally not going to reject patches because of wrongly sorted headers unless there is some real plan behind it. I agree that it might look better. An inverse Christmas' tree also looks better. But it does not mean that it makes the life easier. The important things are still hidden in the details (every single line). From my POV, this patch would just create a mess in the git history and complicate backporting. I am sorry but I will not accept this patch unless there is a wide consensus that this makes sense. Best Regards, Petr
On (23/08/07 16:31), Petr Mladek wrote: > > I am sorry but I will not accept this patch unless there > is a wide consensus that this makes sense. I completely agree with Petr. I found it a little bit hard to be enthusiastic about this patch in particular and _probably_ about this series in general, sorry Andy.
On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote: > On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote: > > Sorting headers alphabetically helps locating duplicates, and > > make it easier to figure out where to insert new headers. > > I agree that includes become a mess after some time. But I am > not persuaded that sorting them alphabetically in random source > files help anything. > > Is this part of some grand plan for the entire kernel, please? > Is this outcome from some particular discussion? > Will this become a well know rule checked by checkpatch.pl? > > I am personally not going to reject patches because of wrongly > sorted headers unless there is some real plan behind it. > > I agree that it might look better. An inverse Christmas' tree > also looks better. But it does not mean that it makes the life > easier. It does from my point of view as maintainability is increased. > The important things are still hidden in the details > (every single line). > > From my POV, this patch would just create a mess in the git > history and complicate backporting. > > I am sorry but I will not accept this patch unless there > is a wide consensus that this makes sense. Your choice, of course, But I see in practice dup headers being added, or some unrelated ones left untouched because header list mess, and in those cases sorting can help (a bit) in my opinion. TL;DR: I was tolerating unsorted mess (for really long header inclusion block) up to the point when I realized how it helps people to maintain the code.
On Mon, Aug 07, 2023 at 11:53:02PM +0900, Sergey Senozhatsky wrote: > On (23/08/07 16:31), Petr Mladek wrote: > > > > I am sorry but I will not accept this patch unless there > > is a wide consensus that this makes sense. > > I completely agree with Petr. > > I found it a little bit hard to be enthusiastic about > this patch in particular and _probably_ about this series > in general, sorry Andy. What to do with _headers_ that include kernel.h for no reason other than sprintf.h (as an example)? Your suggestion, please?
On 07/08/2023 16.58, Andy Shevchenko wrote: > On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote: >> On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote: >>> Sorting headers alphabetically helps locating duplicates, and >>> make it easier to figure out where to insert new headers. >> >> I agree that includes become a mess after some time. But I am >> not persuaded that sorting them alphabetically in random source >> files help anything. >> >> Is this part of some grand plan for the entire kernel, please? >> Is this outcome from some particular discussion? >> Will this become a well know rule checked by checkpatch.pl? >> >> I am personally not going to reject patches because of wrongly >> sorted headers unless there is some real plan behind it. >> >> I agree that it might look better. An inverse Christmas' tree >> also looks better. But it does not mean that it makes the life >> easier. > > It does from my point of view as maintainability is increased. > >> The important things are still hidden in the details >> (every single line). >> >> From my POV, this patch would just create a mess in the git >> history and complicate backporting. >> >> I am sorry but I will not accept this patch unless there >> is a wide consensus that this makes sense. > > Your choice, of course, But I see in practice dup headers being > added, or some unrelated ones left untouched because header list > mess, and in those cases sorting can help (a bit) in my opinion. I agree with Andy on this one. There doesn't need to be some grand master plan to apply this to the entire kernel, but doing it to individual files bit by bit does increase the maintainability. And I really don't buy the backporting argument. Sure, backporting some patch across the release that does the sorting is harder - but then, backporting the sorting patch itself is entirely trivial (maybe not the textual part, but redoing the semantics of it is). _However_, backporting a patch from release z to release y, both of which being later than the release x that did the sorting, is going to be _easier_. It also reduces merge conflicts - that's also why lots of Makefiles are kept sorted. It's of course entirely unrelated to moving the declarations of the provided functions to a separate header file, but IMO both are worth doing. Rasmus
On Mon 2023-08-07 21:47:17, Rasmus Villemoes wrote: > On 07/08/2023 16.58, Andy Shevchenko wrote: > > On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote: > >> On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote: > >>> Sorting headers alphabetically helps locating duplicates, and > >>> make it easier to figure out where to insert new headers. > >> > >> I agree that includes become a mess after some time. But I am > >> not persuaded that sorting them alphabetically in random source > >> files help anything. > >> > >> Is this part of some grand plan for the entire kernel, please? > >> Is this outcome from some particular discussion? > >> Will this become a well know rule checked by checkpatch.pl? > >> > >> I am personally not going to reject patches because of wrongly > >> sorted headers unless there is some real plan behind it. > >> > >> I agree that it might look better. An inverse Christmas' tree > >> also looks better. But it does not mean that it makes the life > >> easier. > > > > It does from my point of view as maintainability is increased. > > > >> The important things are still hidden in the details > >> (every single line). > >> > >> From my POV, this patch would just create a mess in the git > >> history and complicate backporting. > >> > >> I am sorry but I will not accept this patch unless there > >> is a wide consensus that this makes sense. > > > > Your choice, of course, But I see in practice dup headers being > > added, or some unrelated ones left untouched because header list > > mess, and in those cases sorting can help (a bit) in my opinion. > > I agree with Andy on this one. There doesn't need to be some grand > master plan to apply this to the entire kernel, but doing it to > individual files bit by bit does increase the maintainability. And I > really don't buy the backporting argument. Sure, backporting some patch > across the release that does the sorting is harder - but then, > backporting the sorting patch itself is entirely trivial (maybe not the > textual part, but redoing the semantics of it is). _However_, > backporting a patch from release z to release y, both of which being > later than the release x that did the sorting, is going to be _easier_. > It also reduces merge conflicts - that's also why lots of Makefiles are > kept sorted. I am afraid that we will not find a consensus here. I agree that the sorting has some advantage. But I would still like to get some wider agreement on this move from other subsystem. It is a good candidate for a mass change which would be part of some plan. I want to avoid reshuffling this more times according to personal preferences. And I do not want to add this cosmetic subsystem specific requirement. Best Regards, Petr
diff --git a/lib/test_printf.c b/lib/test_printf.c index 7677ebccf3c3..2ab09a0dc841 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -5,24 +5,21 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/bitmap.h> +#include <linux/dcache.h> +#include <linux/gfp.h> +#include <linux/in.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/mm.h> #include <linux/module.h> #include <linux/printk.h> +#include <linux/property.h> #include <linux/random.h> #include <linux/rtc.h> #include <linux/slab.h> -#include <linux/string.h> - -#include <linux/bitmap.h> -#include <linux/dcache.h> #include <linux/socket.h> -#include <linux/in.h> - -#include <linux/gfp.h> -#include <linux/mm.h> - -#include <linux/property.h> +#include <linux/string.h> #include "../tools/testing/selftests/kselftest_module.h" diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 40f560959b16..b17e0744a7bc 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -17,42 +17,44 @@ * - scnprintf and vscnprintf */ -#include <linux/stdarg.h> #include <linux/build_bug.h> #include <linux/clk.h> #include <linux/clk-provider.h> -#include <linux/errname.h> -#include <linux/module.h> /* for KSYM_SYMBOL_LEN */ -#include <linux/types.h> -#include <linux/string.h> +#include <linux/compiler.h> +#include <linux/cred.h> #include <linux/ctype.h> -#include <linux/kernel.h> +#include <linux/dcache.h> +#include <linux/errname.h> +#include <linux/ioport.h> #include <linux/kallsyms.h> +#include <linux/kernel.h> #include <linux/math64.h> -#include <linux/uaccess.h> -#include <linux/ioport.h> -#include <linux/dcache.h> -#include <linux/cred.h> +#include <linux/module.h> /* for KSYM_SYMBOL_LEN */ +#include <linux/notifier.h> +#include <linux/of.h> +#include <linux/property.h> #include <linux/rtc.h> +#include <linux/siphash.h> +#include <linux/stdarg.h> +#include <linux/string.h> +#include <linux/string_helpers.h> #include <linux/time.h> +#include <linux/types.h> +#include <linux/uaccess.h> #include <linux/uuid.h> -#include <linux/of.h> -#include <net/addrconf.h> -#include <linux/siphash.h> -#include <linux/compiler.h> -#include <linux/property.h> -#include <linux/notifier.h> + #ifdef CONFIG_BLOCK #include <linux/blkdev.h> #endif +#include <net/addrconf.h> + #include "../mm/internal.h" /* For the trace_print_flags arrays */ -#include <asm/page.h> /* for PAGE_SIZE */ #include <asm/byteorder.h> /* cpu_to_le16 */ +#include <asm/page.h> /* for PAGE_SIZE */ #include <asm/unaligned.h> -#include <linux/string_helpers.h> #include "kstrtox.h" /* Disable pointer hashing if requested */
Sorting headers alphabetically helps locating duplicates, and make it easier to figure out where to insert new headers. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- lib/test_printf.c | 17 +++++++---------- lib/vsprintf.c | 38 ++++++++++++++++++++------------------ 2 files changed, 27 insertions(+), 28 deletions(-)