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