Bug 124820 – check-webkit-style should check member initialization indentation

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brian Burg]

The solution should be added in the vicinity of Source/Tools/Scripts/webkitpy/style/checkers/cpp.py, in function check_spacing.

[László Langó]

(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?

[Brian Burg]

(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.

[László Langó]

Created attachment 217932 [details]
Patch

[Peter Gal]

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.

[László Langó]

(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.

[Peter Gal]

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.

[Zoltan Herczeg]

Comment on attachment 217932 [details]
Patch

Nice python magic. r=me

[WebKit Commit Bot]

Comment on attachment 217932 [details]
Patch

Clearing flags on attachment: 217932

Committed r160084: <http://trac.webkit.org/changeset/160084>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Burg]

Great work, thanks for picking these up!