Bug 124820

Summary: check-webkit-style should check member initialization indentation
Product: WebKit Reporter: Brian Burg <burg>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, darin, glenn, llango.u-szeged, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.webkit.org/coding/coding-style.html#punctuation-member-init
Attachments:
Description Flags
Patch none

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!