Bug 40724 – Check for extra spacing on a variable declaration line.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sam Magnuson]

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.

[Sam Magnuson]

Created attachment 58910 [details]
Proposed patch

[Shinichiro Hamaji]

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.

[Sam Magnuson]

Created attachment 59016 [details]
Proposed patch

[Sam Magnuson]

(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.

[Shinichiro Hamaji]

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.

[Sam Magnuson]

Created attachment 59437 [details]
Rediff against trunk

[WebKit Review Bot]

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.

[Shinichiro Hamaji]

Comment on attachment 59437 [details]
Rediff against trunk

Looks good. I'll land this with fixing the style error.

[Shinichiro Hamaji]

Committed r61660: <http://trac.webkit.org/changeset/61660>

[Shinichiro Hamaji]

(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.