[U-Boot] Policy for checkpatch usage?

[Detlev Zundel <dzu@denx.de>]

Hi Andreas,

> Am Donnerstag, den 21.04.2011, 17:46 +0200 schrieb Detlev Zundel:
>> Thanks for the input.  Current base for discussion:
>> 
>>   Checkpatch is a tool that can help you find some style problems, but
>>   is imperfect, and the things it complains about are of varying
>>   importance.  So use common sense in interpreting the results.
>> 
>>   Warnings that clearly only make sense in the Linux kernel can be
>>   ignored.  This includes "Use #include <linux/$file> instead of
>>   <asm/$file>" for example.
>>   
>>   If you encounter warnings for existing code, not modified by your
>>   patch, consider submitting a separate, cosmetic-only patch -- clearly
>>   described as such -- that *precedes* your substantive patch.
>
> For minor modifications (e.g. changed arguments of a function call),
> adhere to the present codingstyle of the module. Relating checkpatch
> warnings can be ignored in this case. A respective note in the commit or
> cover letter why they are ignored is desired.
>
>
>> Anyone unhappy with that?  Will this help newcomers?
>
> Sounds sensible to me. The extra paragraph above is to circumvent
> intermixing codingstyle inside a file. For example in common/main.c,
> there are "function (arg, arg)" all around. Not checkpatch compliant
> (and ugly, IMHO). Now when I modify one of this calls, e.g add an
> argument, I'd have to change this. Which leads to a mixture of styles in
> the file, which is even worse than than adhering to the "broken" style.

Thanks, I've updated the wiki page accordingly[1].

> In general, I'd suggest a - maybe informal - policy that while
> codingstyle is important, there are more relevant aspects like elegant
> code, sensible interfaces, proper use of static and const and so on.
> I'm sure any developer is willing to debate about these issues, but most
> volunteers (esp. company or freelance coders) will resign after the 3rd
> round of whitespace ping-pong...

I fully agree, but on the other hand if we do have a Codingstyle
document _and_ a tool to check for it, there should be no more than one
round and even this one round should only be neccessary if the poster
didn't previously read the Codingstyle document ;)

> Not sure how to phrase that without contradicting the basic idea, maybe
> with an additional paragraph like:
> "New modules have to be as checkpatch compliant as possible. Violations
> have to be justified in the commit or cover letter."

Let's skip this.  My experience is that people want clear and sharp-cut
policies so that they can decide for themselves _before_ posting whether
they adhere to them.  If we cannot achieve such a strictness in our
forumlation, I expect such clauses to generate more discussion than they
solve.

Thanks everyone for the input!
  Detlev

[1] http://www.denx.de/wiki/U-Boot/Patches

-- 
System going down at 1:45 this afternoon for disk crashing.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de