Bug 40724 - Check for extra spacing on a variable declaration line.
Summary: Check for extra spacing on a variable declaration line.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-16 11:20 PDT by Sam Magnuson
Modified: 2010-06-23 00:00 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (2.80 KB, patch)
2010-06-16 11:26 PDT, Sam Magnuson
hamaji: review-
Details | Formatted Diff | Diff
Proposed patch (3.07 KB, patch)
2010-06-17 11:04 PDT, Sam Magnuson
hamaji: review+
Details | Formatted Diff | Diff
Rediff against trunk (2.76 KB, patch)
2010-06-22 16:12 PDT, Sam Magnuson
hamaji: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Magnuson 2010-06-16 11:20:37 PDT
I've been bitten before by having:

struct {
   int     a;
   int     b;
}

As my previous guidelines had me working that way. I've had reviews ask that I fix this, and it would have been great to have check-webkit-style flag this for me to prevent wasting reviewer time.
Comment 1 Sam Magnuson 2010-06-16 11:26:27 PDT
Created attachment 58910 [details]
Proposed patch
Comment 2 Shinichiro Hamaji 2010-06-16 23:03:49 PDT
Comment on attachment 58910 [details]
Proposed patch

Thanks for this patch! This feature is very useful, but I think we can improve this.

Two general comments:

- You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change.
- Please create a patch to the newest tree. I think cpp.py was moved from processors directory to checkers directory.

WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
 +      matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
This regexp will catch "else  if" as well, and this is useful warning. So, I think we should change <type> and <name> to <token1> and <token2> or something like this.

WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
 +      matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
I think it would be nice if we can check "int*  a", "int&  a", "if  (", etc.

WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368
 +                'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) )
Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space.

So, I would say "Extra space between '%s' and '%s'" or something like this.
Comment 3 Sam Magnuson 2010-06-17 11:04:32 PDT
Created attachment 59016 [details]
Proposed patch
Comment 4 Sam Magnuson 2010-06-17 11:06:01 PDT
(In reply to comment #2)
> (From update of attachment 58910 [details])
> Thanks for this patch! This feature is very useful, but I think we can improve this.
> 
> Two general comments:
> 
> - You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change.

This seemed to pass for me, it even spotted a handful of existing errors :)

> - Please create a patch to the newest tree. I think cpp.py was moved from processors directory to checkers directory.
> 
> WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
>  +      matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
> This regexp will catch "else  if" as well, and this is useful warning. So, I think we should change <type> and <name> to <token1> and <token2> or something like this.
> 

Fair enough, I've done this - add added the 'else  if' to the unittest. 

> WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
>  +      matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
> I think it would be nice if we can check "int*  a", "int&  a", "if  (", etc.
>

Ok, I added this and put it in the unit test as well.
 
> WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368
>  +                'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) )
> Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space.
> 
> So, I would say "Extra space between '%s' and '%s'" or something like this.

Agreed, changed.
Comment 5 Shinichiro Hamaji 2010-06-17 21:43:02 PDT
Comment on attachment 59016 [details]
Proposed patch

Looks great. Thanks for your quick update.

> > - You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change.
> 
> This seemed to pass for me, it even spotted a handful of existing errors :)

Nice! Thanks for your checking.
Comment 6 Sam Magnuson 2010-06-22 16:12:18 PDT
Created attachment 59437 [details]
Rediff against trunk
Comment 7 WebKit Review Bot 2010-06-22 16:15:09 PDT
Attachment 59437 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1375:  whitespace before ')'  [pep8/E202] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Shinichiro Hamaji 2010-06-22 21:25:29 PDT
Comment on attachment 59437 [details]
Rediff against trunk

Looks good. I'll land this with fixing the style error.
Comment 9 Shinichiro Hamaji 2010-06-22 23:30:44 PDT
Committed r61660: <http://trac.webkit.org/changeset/61660>
Comment 10 Shinichiro Hamaji 2010-06-23 00:00:47 PDT
(In reply to comment #9)
> Committed r61660: <http://trac.webkit.org/changeset/61660>

It looks like the test doesn't success with your last patch because you forgot to modify the test expectations after you addressed the following comment. It's a very good idea to run WebKitTools/Scripts/test-webkit-scripts before you upload your patch when you modify our scripts. Anyway, thanks again for your contribution!

> WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368
>  +                'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) )
> Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space.
> 
> So, I would say "Extra space between '%s' and '%s'" or something like this.