Let them run CAKE
Let them run CAKE
Posted Jun 28, 2018 13:38 UTC (Thu) by imMute (guest, #96323)Parent article: Let them run CAKE
>the inevitable requirement to put variable declarations in "reverse Christmas tree" ordering. From the linked mail:
> +static void cake_heapify(struct cake_sched_data *q, u16 i)
> +{
> + static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
> + u32 m = i;
> + u32 mb = cake_heap_get_backlog(q, m);
Please order local variables from longest to shortest line.
I.... what?! Is that an actual code style guideline? In the code above, you can't simply reorder the lines: the initialization of mb uses m, so reordering them blindly will fail to compile. Of course, in this case, it can simply be fixed by changing which variable is passed into the function, but other cases like this might not be able to. Imagine if the initialization of m was more complicated, but not quite as long as the initialization of mb. Seems like a semi-pointless rule (I say "semi-pointless", because if there's no problem with a specific case, then do it anyway for readability.)
Posted Jun 28, 2018 13:47 UTC (Thu)
by mtaht (subscriber, #11087)
[Link] (4 responses)
Posted Jun 28, 2018 14:48 UTC (Thu)
by corbet (editor, #1)
[Link] (2 responses)
There's been a low rumble of conversation for a bit about trying to document the subsystem-specific rules — both to make them easier to follow and to simply shine some sunlight on some of them. But we've not managed to make any progress on that yet.
Posted Jun 28, 2018 22:53 UTC (Thu)
by gerdesj (subscriber, #5446)
[Link]
"One of the many obstacles we put in the path of incoming kernel developers is the fact that different subsystems have different rules, and many of them are not written down."
Perhaps those subsystems might like to update checkpatch.pl if they require additional "rules" not already covered by Mr T for the whole project. Surely a maintainer could save loads of time by investing a few hours with perl n git *once* and not have to keep NACKing stuff with style snags, *forever* P)
Posted Jun 29, 2018 2:39 UTC (Fri)
by marcH (subscriber, #57642)
[Link]
As often with code style questions, either extreme position (strict enforcement or not caring at all) is too simplistic and wrong. This rule does make the code easier to read but other considerations like dependencies or logical grouping should be able to outweigh it easily. I believe checkpatch documentation has a good warning against robotic enforcement.
PS: one-letter variables, really? This function is barely readable unless you're an expert (and even then I'm not sure)
Posted Jun 28, 2018 17:06 UTC (Thu)
by zdzichu (subscriber, #17118)
[Link] (1 responses)
Posted Jun 29, 2018 14:04 UTC (Fri)
by mtaht (subscriber, #11087)
[Link]
Posted Jul 3, 2018 14:08 UTC (Tue)
by robbe (guest, #16131)
[Link]
Let them run CAKE
One of the many obstacles we put in the path of incoming kernel developers is the fact that different subsystems have different rules, and many of them are not written down. The cosmetic ordering of local variable declarations is one of those; only a few maintainers insist on that, while most really do not care or think that it's actively silly. There would probably be a fair amount of opposition to enshrining this requirement in checkpatch.pl.
Reverse Christmas trees
Reverse Christmas trees
Reverse Christmas trees
Let them run CAKE
Let them run CAKE
If readability is really the only reason¹, you could always replace the three lines with
reverse christmas tree
static const u32 a = CAKE_MAX_TINS * CAKE_QUEUES;
u32 m = i; /* this comment pleases the eye */
u32 mb = cake_heap_get_backlog(q, m);
¹ all rules should have an attached rationale