Re: [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines

Hi Doug,

On Thu, Mar 21, 2013 at 9:35 AM, Doug Anderson dianders@chromium.org wrote:

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:

...

checkpatch has a new type of warning, a 'CHECK'. At present patman fails with these, which makes it less than useful.

Add support for checks, making it backwards compatible with the old checkpatch.

There are also a few other minor fixups in this:

• Cleanup formatting of the CheckPatches() output.
• Fix erroneous "internal error" if multiple patches have warnings.
• Be more robust to new types of problems.

Yes - I'll add them to the commit message.

...

@@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False): 'msg': text message 'file' : filename 'line': line number

•        error_count: Number of errors
  

•        warning_count: Number of warnings
  

•        check_count: Number of checks
         lines: Number of lines
  

•        stdout: Full output of checkpatch
  

nit: Right above this it says you're returning a 4-tuple. That's no longer true. Could just remove the "4-".

optional: I've found that returning big tuples like this is problematic. Whenever you change the number of things returned you've got to modify any callers that call like:

a, b, c, d = CheckPatch(...)

to now be:

a, b, c, d, e = CheckPatch(...)

...or never use the above syntax and use:

result = CheckPatch(...) blah = result[0]

Maybe use a namedtuple so that callers can use the result more cleanly?

Yes, we are well past the point of needing something like that, so will add it.

...

     if match:
         error_count = int(match.group(1))
         warning_count = int(match.group(2))

•        lines = int(match.group(3))
  

•        match_count = int(match.group(3))
  

•        if len(match.groups()) == 4:
  

•           check_count = match_count
  

•           lines = int(match.group(4))
  

I'm confused about match_count here. What is it supposed to contain? I can't tell from the name of it. It looks like a temporary variable holding either check_count or lines. ...but you forget to assign "lines = match_count" in an "else" case so things are broken with old versions of checkpatch, right?

Yes - we don't currently use the return value. But I will fix it.

Regards, Simon

-Doug