Bug 27984 – cpp_style.py lacks checks for pointer and reference declaration style.

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Fenton]

Based on webkit style guidelines the following pointer declaration is correct.

TypeName* variableName;

where as TypeName *variableName; is not.

The same applies to reference.

cpp_style.py requires an update to correct test for these two style guidelines.

[Mike Fenton]

Created attachment 34072 [details]
Patch for cpp_style.py to check for pointer and reference marker placement.

Patch checks for 

TypeName *variableName.

In order to eliminate false positives, and added check to see if an = sign comes before the * or & has been added to eliminate matches like.

int a = b * c;

[David Levin]

It would be good to run over the current code base (or at least WebCore/dom, WebCore/css) to check for false positives.

Maybe it should use parenthesis also as a clue.

   myFunction(a * b);

Of course, there is the counter example,
   for (int* a = ptr;...

but I suspect this is more rare.

[David Levin]

Comment on attachment 34072 [details]
Patch for cpp_style.py to check for pointer and reference marker placement.

This needs unit tests (added to cpp_style_unittests.py)

Also, you could combine the regex and put them in a (named) group and then use the result from the group in the printed error message.

Lastly, this guideline only applies to C++ code.  The tool also runs on C files and the guideline is reversed in that case.  (int *i;)

[Mike Fenton]

Created attachment 34137 [details]
Updated patch for cpp_style validation of pointer / references.

Patch has been updated with the following chanages.

1) Add unit tests.
2) Update unit test for c style cast to be processed as c style code (ie. don't complain about the int *b)
3) Divide logic for C/C++ validation to properly support each style.
4) Tighten up logic of regex to compare to beginnings of lines only (greatly reduces false matches)
5) Add extra check for references to avoid return &variable; matching.

[David Levin]

Comment on attachment 34137 [details]
Updated patch for cpp_style validation of pointer / references.

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;

[Mike Fenton]

Created attachment 34209 [details]
Revised Patch as per suggestions

Patch as been updated as per the great suggestions laid out.

[David Levin]

Comment on attachment 34209 [details]
Revised Patch as per suggestions

Your changelog entry is not at the top of the file.  Whoever lands this should fix that.

[Eric Seidel (no email)]

Comment on attachment 34209 [details]
Revised Patch as per suggestions

svn-apply will re-order the ChangeLog automagically.  Or at least it should.

[Adam Barth]

Comment on attachment 34209 [details]
Revised Patch as per suggestions

Clearing review flag on attachment: 34209

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/modules/cpp_style.py
	M	WebKitTools/Scripts/modules/cpp_style_unittest.py
Committed r46856
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/modules/cpp_style_unittest.py
	M	WebKitTools/Scripts/modules/cpp_style.py
r46856 = e78f2575f21c264c00051538030f87c07339b1f8 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46856

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[David Levin]

> svn-apply will re-order the ChangeLog automagically.  Or at least it should.

Unfortunately, it doesn't seem to http://trac.webkit.org/changeset/46856

[Eric Seidel (no email)]

(In reply to comment #11)
> > svn-apply will re-order the ChangeLog automagically.  Or at least it should.
> 
> Unfortunately, it doesn't seem to http://trac.webkit.org/changeset/46856

Sad.  bug 26865 and bug 27355 cover this.