[U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet

[Albert ARIBAUD]

Le 14/04/2011 08:12, Mike Frysinger a écrit :
> On Thursday, April 14, 2011 01:58:32 Albert ARIBAUD wrote:
>> Le 14/04/2011 01:30, Mike Frysinger a écrit :
>>> On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
>>>> btw. I suspect the change is to keep checkpatch.pl happy about the line
>>>> length.
>>>
>>> also, checkpatch is a tool in the toolbox.  people should not be blindly
>>> following it, but reviewing its output to see what should be changed and
>>> which should be ignored.
>>>
>>> if checkpatch is complaining about code that you arent changing, then you
>>> probably shouldnt worry about it.  especially when the only thing you're
>>> doing is changing style.
>>
>> I tend to see this "don't worry about some checkpatch.pl messages"
>> appraoch as similar to "don't worry about some C compiler warnings". in
>> that indeed "you probably shouldn't worry about it", and the key is
>> "probably": when it bites you back later on, you realize you "probably"
>> should have worried. If you apply a zero-C-warning policy, then a
>> zero-checkpatch-warning policy makes sense as well...
>
> how about when it's plain wrong ?  or it's applying a rule that (most of the
> time) is correct, but not *all* the time ?  or it complains about code that
> your patch isnt touching (as is the case here) ?  or it complains about code
> that is being imported (from linux or other projects) ?

As for "plain wrong" warnings, I think this was addressed in the two 
paragraphs of my answer which followed the one you quote here. :)

As for rule exceptions, nothing prohibits submitters from stating the 
number of checkpatch warnings in their patch and justifying why they did 
not resolve them.

As for code the patch is not touching, again, it could be one of these 
established exceptions -- but again, it has to be a global decision so 
that reviewers don't emit contradicting requests about it.

As for code imported from Linux or any other project, there is an 
exception already, right on the second paragraph of the Wiki page on 
coding style <http://www.denx.de/wiki/U-Boot/CodingStyle>.

> so i stand by my statement that checkpatch is a tool and does *not* get the
> final say.  blindly following a tool is good -- if you're blind.

When it emits warnings, the C toolchain is also just a tool and does 
also not have the final say ; that's why they are warnings and not 
errors, because they might indicate something wrong, or not.

Besides, the coding rules for U-Boot make the tool checkpatch.pl 
mandatory, and I don't think it means "run it then don't care if some 
warnings are there"; I think it means "run it and make sure you 
eliminate all warnings if you can, and if there are some left, either 
justify them when you submit the patch, or ask about them on the list".

As for the 'blind' part, please re-read the two paragraphs right after 
the one you commented here; I believe it makes it clear that I do not 
advocate blindness, but on the contrary, strictness in which warnings we 
would agree to turn a blind eye to, and which we would not.

> -mike

Anyway, I think we've both made our opinions clear; to avoid spending 
too much time here, I guess the best is that Wolfgang, who has the final 
say :), states exactly how he intends submitters and reviewers to handle 
checkpatch diagnostics.

Amicalement,
-- 
Albert.