On Fri, 2013-06-07 at 22:42 +0200, Thomas Gleixner wrote:
> On Thu, 6 Jun 2013, Kefeng Wang wrote:
>
> > Fix all the checkpath errors in kernel/irq dir, and some warnings
> > also fixed.
>
> Sorry, I'm not really interested in this kind of patches. To be
> honest, it would be way more exciting if you had taught checkpatch to
> actually fix the missing space after the comma.
Yup. Kefeng, if you want to take that up,
I'd be happy to help/guide you.
> Aside of that your mechanical fixups are mostly making the code worse
> to read. Just a few examples:
>
> --- linux-2.6.orig/kernel/irq/chip.c
[]
> struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
>
> IRQ_GET_DESC_CHECK_GLOBAL);
>
> Aligning the first arguments of the first and the second line makes it
> way simpler to read.
or maybe use
struct irq_desc *desc =
irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
and satisfy 80 cols too.
Existing uses that exceed the 80 column text limit shouldn't
be changed without making other useful changes at the same
time. Make 80 column changes as part of a larger patch set,
> --- linux-2.6.orig/kernel/irq/handle.c
[]
> @@ -47,7 +47,7 @@ static void warn_no_thread(unsigned int
> - printk(KERN_WARNING "IRQ %d device %s returned IRQ_WAKE_THREAD "
> + pr_warn("IRQ %d device %s returned IRQ_WAKE_THREAD "
> "but no thread function available.", irq, action->name);
It'd be useful to add a terminating \n though to avoid
interleaving by other thread pr_cont/naked printks.
> The checkpatch warning is to prevent new code to use the old style
> printk format, but it's not intended to force that on existing code.
Yup, and checkpatch will never be a perfect tool.
> Please sit down and read and understand the code and try to find real
> issues which cannot be detected and solved by scripts and
> tools. That's what's kernel hacking is about. You are not becoming a
> kernel developer by running tools and blindly fixing the complaints of
> the tools. You have to understand the code and you have to learn to
> judge the output of tools.
Quite right.
Maybe add something like it to SubmittingPatches in
Section 2 ala:
5) "Don't be a checkpatch monkey - How not to write patches"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
