On 2013-06-08 6:24, Joe Perches wrote:
> 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"
>
hi Thomas and Joe， thank very much for your reply and remind,
I will pay more attention to study kernel， like your said,
read more code and be an good kernel developer.
thanks,
kefeng
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
