[webkit-reviews] review denied: [Bug 40724] Check for extra spacing on a variable declaration line. : [Attachment 58910] Proposed patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Sam Magnuson
<smagnuson at netflix.com>'s request for review:
Bug 40724: Check for extra spacing on a variable declaration line.
https://bugs.webkit.org/show_bug.cgi?id=40724

Attachment 58910: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=58910&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
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.