[Webkit-unassigned] [Bug 27984] cpp_style.py lacks checks for pointer and reference declaration style.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27984


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34137|review?                     |review-
               Flag|                            |




--- Comment #5 from David Levin <levin at chromium.org>  2009-08-06 00:39:21 PDT ---
(From update of attachment 34137)
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;

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.