diff mbox

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

Message ID 1363833781-14557-2-git-send-email-sjg@chromium.org
State Superseded, archived
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass March 21, 2013, 2:42 a.m. UTC 
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.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/checkpatch.py | 75 +++++++++++++++++++++++++++++-----------------
 tools/patman/test.py       |  9 ++++--
 2 files changed, 54 insertions(+), 30 deletions(-)

Comments

Doug Anderson March 21, 2013, 4:35 p.m. UTC | #1 
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.


> @@ -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?


>          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?


-Doug
Simon Glass March 26, 2013, 10:12 p.m. UTC | #2 
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
diff mbox

Patch

diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index d3a0477..d1743ae 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -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
     """
     result = False
-    error_count, warning_count, lines = 0, 0, 0
+    error_count, warning_count, check_count, lines = 0, 0, 0, 0
     problems = []
     chk = FindCheckPatch()
     item = {}
@@ -76,11 +80,16 @@  def CheckPatch(fname, verbose=False):
     #stdout, stderr = pipe.communicate()
 
     # total: 0 errors, 0 warnings, 159 lines checked
+    # or:
+    # total: 0 errors, 2 warnings, 7 checks, 473 lines checked
     re_stats = re.compile('total: (\\d+) errors, (\d+) warnings, (\d+)')
+    re_stats_full = re.compile('total: (\\d+) errors, (\d+) warnings, (\d+)'
+                               ' checks, (\d+)')
     re_ok = re.compile('.*has no obvious style problems')
     re_bad = re.compile('.*has style problems, please review')
     re_error = re.compile('ERROR: (.*)')
     re_warning = re.compile('WARNING: (.*)')
+    re_check = re.compile('CHECK: (.*)')
     re_file = re.compile('#\d+: FILE: ([^:]*):(\d+):')
 
     for line in stdout.splitlines():
@@ -91,29 +100,39 @@  def CheckPatch(fname, verbose=False):
         if not line and item:
             problems.append(item)
             item = {}
-        match = re_stats.match(line)
+        match = re_stats_full.match(line)
+        if not match:
+            match = re_stats.match(line)
         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))
         elif re_ok.match(line):
             result = True
         elif re_bad.match(line):
             result = False
-        match = re_error.match(line)
-        if match:
-            item['msg'] = match.group(1)
+        err_match = re_error.match(line)
+        warn_match = re_warning.match(line)
+        file_match = re_file.match(line)
+        check_match = re_check.match(line)
+        if err_match:
+            item['msg'] = err_match.group(1)
             item['type'] = 'error'
-        match = re_warning.match(line)
-        if match:
-            item['msg'] = match.group(1)
+        elif warn_match:
+            item['msg'] = warn_match.group(1)
             item['type'] = 'warning'
-        match = re_file.match(line)
-        if match:
-            item['file'] = match.group(1)
-            item['line'] = int(match.group(2))
+        elif check_match:
+            item['msg'] = check_match.group(1)
+            item['type'] = 'check'
+        elif file_match:
+            item['file'] = file_match.group(1)
+            item['line'] = int(file_match.group(2))
 
-    return result, problems, error_count, warning_count, lines, stdout
+    return (result, problems, error_count, warning_count, check_count,
+            lines, stdout)
 
 def GetWarningMsg(col, msg_type, fname, line, msg):
     '''Create a message for a given file/line
@@ -128,37 +147,39 @@  def GetWarningMsg(col, msg_type, fname, line, msg):
         msg_type = col.Color(col.YELLOW, msg_type)
     elif msg_type == 'error':
         msg_type = col.Color(col.RED, msg_type)
+    elif msg_type == 'check':
+        msg_type = col.Color(col.MAGENTA, msg_type)
     return '%s: %s,%d: %s' % (msg_type, fname, line, msg)
 
 def CheckPatches(verbose, args):
     '''Run the checkpatch.pl script on each patch'''
-    error_count = 0
-    warning_count = 0
+    error_count, warning_count, check_count = 0, 0, 0
     col = terminal.Color()
 
     for fname in args:
-        ok, problems, errors, warnings, lines, stdout = CheckPatch(fname,
-                verbose)
+        ok, problems, errors, warnings, checks, lines, stdout = CheckPatch(
+                fname, verbose)
         if not ok:
             error_count += errors
             warning_count += warnings
-            print '%d errors, %d warnings for %s:' % (errors,
-                    warnings, fname)
-            if len(problems) != error_count + warning_count:
+            check_count += checks
+            print '%d errors, %d warnings, %d checks for %s:' % (errors,
+                    warnings, checks, col.Color(col.BLUE, fname))
+            if len(problems) != errors + warnings + checks:
                 print "Internal error: some problems lost"
             for item in problems:
-                print GetWarningMsg(col, item['type'],
+                print GetWarningMsg(col, item.get('type', '<unknown>'),
                         item.get('file', '<unknown>'),
-                        item.get('line', 0), item['msg'])
+                        item.get('line', 0), item.get('msg', 'message'))
+            print
             #print stdout
-    if error_count != 0 or warning_count != 0:
-        str = 'checkpatch.pl found %d error(s), %d warning(s)' % (
-            error_count, warning_count)
+    if error_count or warning_count or check_count:
+        str = 'checkpatch.pl found %d error(s), %d warning(s), %d checks(s)'
         color = col.GREEN
         if warning_count:
             color = col.YELLOW
         if error_count:
             color = col.RED
-        print col.Color(color, str)
+        print col.Color(color, str % (error_count, warning_count, check_count))
         return False
     return True
diff --git a/tools/patman/test.py b/tools/patman/test.py
index f801ced..5a41ef5 100644
--- a/tools/patman/test.py
+++ b/tools/patman/test.py
@@ -218,7 +218,8 @@  index 0000000..2234c87
     def testCheckpatch(self):
         """Test checkpatch operation"""
         inf = self.SetupData('good')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, True)
         self.assertEqual(problems, [])
         self.assertEqual(err, 0)
@@ -227,7 +228,8 @@  index 0000000..2234c87
         os.remove(inf)
 
         inf = self.SetupData('no-signoff')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, False)
         self.assertEqual(len(problems), 1)
         self.assertEqual(err, 1)
@@ -236,7 +238,8 @@  index 0000000..2234c87
         os.remove(inf)
 
         inf = self.SetupData('spaces')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, False)
         self.assertEqual(len(problems), 2)
         self.assertEqual(err, 0)