[webkit-reviews] review denied: [Bug 27984] cpp_style.py lacks checks for pointer and reference declaration style. : [Attachment 34137] Updated patch for cpp_style validation of pointer / references.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Mike Fenton
<mike.fenton at torchmobile.com>'s request for review:
Bug 27984: cpp_style.py lacks checks for pointer and reference declaration
style.
https://bugs.webkit.org/show_bug.cgi?id=27984

Attachment 34137: Updated patch for cpp_style validation of pointer /
references.
https://bugs.webkit.org/attachment.cgi?id=34137&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Although less comprehensive, it has far less false positives than before which
is awesome.

Just a few comments to consider and then we'll be all done!

> diff --git a/WebKitTools/Scripts/modules/cpp_style.py
b/WebKitTools/Scripts/modules/cpp_style.py
> +    if filename.endswith('.cpp'):
> +	   # C++ Pointer declaration should have the * beside the type not the
variable name.
> +	   matched = search(r'^\s*\w+\s\*\w+', line)

It seems like this should have the return in its as well. 
Also match works well here since this always starts at the beginning of the
string.
The second \s would match more if it were \s+

Or in code (added a word break before the return)
       matched = match(r'\s*\w+(?<!\breturn)\s+\*\w+', line)


> +	   if matched:
> +	       error(filename, line_number, 'whitespace/declaration', 3,
> +		     'Pointer declaration has space between type name and * (
in %s' % matched.group(0).strip())

I don't understand the "(" in error message.

All of these comments apply to the "&" block as well, so it just seems better
if it were the same code like this:

       # C++ should have the & or * beside the type not the variable name.
       matched = match(r'\s*\w+(?<!\breturn)\s+(?P<pointer_operator>\*|\&)\w+',
line)
       if matched:
	   error(filename, line_number, 'whitespace/declaration', 3,
		 'Reference declaration has space between type name and %s in
%s' % (matched.group('pointer_operator'), matched.group(0).strip()))
> +
> +    elif filename.endswith('.c'): 
> +	   # C Pointer declaration should have the * beside the variable not
the type name.
> +	   matched = search(r'^\s*\w+\*\s\w+', line)

Suggested:
     matched = match(r'\s*\w+\*\s+\w+', line)

> +	   if matched:
> +	       error(filename, line_number, 'whitespace/declaration', 3,
> +		     'Pointer declaration has space between * and variable
name( in %s' % matched.group(0).strip())

I don't understand the "(" in error message.

> diff --git a/WebKitTools/Scripts/modules/cpp_style_unittest.py
b/WebKitTools/Scripts/modules/cpp_style_unittest.py

Please add test for return *b;