Bug 124820 - check-webkit-style should check member initialization indentation
Summary: check-webkit-style should check member initialization indentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.webkit.org/coding/coding-s...
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-24 17:59 PST by Brian Burg
Modified: 2013-12-04 09:06 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.79 KB, patch)
2013-11-27 02:33 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2013-11-24 17:59:55 PST
The solution should be added in the vicinity of Source/Tools/Scripts/webkitpy/style/checkers/cpp.py, in function check_spacing.
Comment 1 László Langó 2013-11-25 04:43:06 PST
(In reply to comment #0)
> The solution should be added in the vicinity of Source/Tools/Scripts/webkitpy/style/checkers/cpp.py, in function check_spacing.

Are you working on the fix, or just reported the bug?
Comment 2 Brian Burg 2013-11-25 08:05:59 PST
(In reply to comment #1)
> (In reply to comment #0)
> > The solution should be added in the vicinity of Source/Tools/Scripts/webkitpy/style/checkers/cpp.py, in function check_spacing.
> 
> Are you working on the fix, or just reported the bug?

Reported only, with some hints in case I decide to look at it sometime later.
Comment 3 László Langó 2013-11-27 02:33:50 PST
Created attachment 217932 [details]
Patch
Comment 4 Peter Gal 2013-11-28 09:09:29 PST
Comment on attachment 217932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217932&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1979
> +    if search(r'\b([A-Za-z0-9_]*_)\(\1\)', line):

Why is there an underscore in the end of the first group?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1997
> +    if search(r'(?P<indentation>\s*)([^\s]\(.*\)\s?\:|^\s*\:).*[^;]*$', line):
> +        if search(r'[^:]\:[^\:\s]+', line):
> +            error(line_number, 'whitespace/init', 4,
> +                'Missing spaces around :')
> +        if search(r'[^\s]\(.*\)\s?\:.*[^;]*$', line):
> +            error(line_number, 'whitespace/indent', 4,
> +                'Should be indented on a separate line, with the colon or comma first on that line.')
> +        else:
> +            begin_line, begin_line_number = get_previous_non_blank_line(clean_lines, line_number)
> +
> +        matched = search(r'(?P<indentation>\s*).*', begin_line)

For this 'matched 'you could use the regex's return value in the if above.
Comment 5 László Langó 2013-11-28 23:10:42 PST
(In reply to comment #4)
> (From update of attachment 217932 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217932&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:1979
> > +    if search(r'\b([A-Za-z0-9_]*_)\(\1\)', line):
> 
> Why is there an underscore in the end of the first group?

I don't know, that is not my code. I just moved it into this new method, because this functionality should be here.
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:1997
> > +    if search(r'(?P<indentation>\s*)([^\s]\(.*\)\s?\:|^\s*\:).*[^;]*$', line):
> > +        if search(r'[^:]\:[^\:\s]+', line):
> > +            error(line_number, 'whitespace/init', 4,
> > +                'Missing spaces around :')
> > +        if search(r'[^\s]\(.*\)\s?\:.*[^;]*$', line):
> > +            error(line_number, 'whitespace/indent', 4,
> > +                'Should be indented on a separate line, with the colon or comma first on that line.')
> > +        else:
> > +            begin_line, begin_line_number = get_previous_non_blank_line(clean_lines, line_number)
> > +
> > +        matched = search(r'(?P<indentation>\s*).*', begin_line)
> 
> For this 'matched 'you could use the regex's return value in the if above.

No it's not the same. The base of the indention can be the previous non blank line. example:

    myClass()
        : myInt(0)
    { }

The above if you mentioned will match on the second line, but we need the indention of the first line.
Comment 6 Peter Gal 2013-11-29 01:15:46 PST
Comment on attachment 217932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217932&action=review

>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1997
>>> +        matched = search(r'(?P<indentation>\s*).*', begin_line)
>> 
>> For this 'matched 'you could use the regex's return value in the if above.
> 
> No it's not the same. The base of the indention can be the previous non blank line. example:
> 
>     myClass()
>         : myInt(0)
>     { }
> 
> The above if you mentioned will match on the second line, but we need the indention of the first line.

Oh.. I missed the second parameter of the search. My bad.
Comment 7 Zoltan Herczeg 2013-12-04 02:51:25 PST
Comment on attachment 217932 [details]
Patch

Nice python magic. r=me
Comment 8 WebKit Commit Bot 2013-12-04 03:08:47 PST
Comment on attachment 217932 [details]
Patch

Clearing flags on attachment: 217932

Committed r160084: <http://trac.webkit.org/changeset/160084>
Comment 9 WebKit Commit Bot 2013-12-04 03:08:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Brian Burg 2013-12-04 09:06:47 PST
Great work, thanks for picking these up!