Bug 97602 – C++ style checker should warn when the indentation is wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

C++ style checker should warn when the indentation is wrong

[Tony Chang]

Created attachment 165672 [details]
Patch

[Tony Chang]

Spin off from https://bugs.webkit.org/show_bug.cgi?id=95930#c7 .

I tried running CSSParser.h and CSSParser.cpp to find spurious warnings, but I may have missed some cases.

[David Levin]

Comment on attachment 165672 [details]
Patch

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

> Tools/ChangeLog:8
> +        Rewrite the indentation checker to ensure that indentation is always a factor of 4

Is that always true? 

I thought WebKit code did parenthesis alignment on continued lines
  functionName(blah,...
               blah, blah, blah);

[Tony Chang]

Comment on attachment 165672 [details]
Patch

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

>> Tools/ChangeLog:8
>> +        Rewrite the indentation checker to ensure that indentation is always a factor of 4
> 
> Is that always true? 
> 
> I thought WebKit code did parenthesis alignment on continued lines
>   functionName(blah,...
>                blah, blah, blah);

That was the specific case that Darin said was not allowed.  See https://bugs.webkit.org/attachment.cgi?id=165498&action=review .

[David Levin]

(In reply to comment #4)
> (From update of attachment 165672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165672&action=review
> 
> >> Tools/ChangeLog:8
> >> +        Rewrite the indentation checker to ensure that indentation is always a factor of 4
> > 
> > Is that always true? 
> > 
> > I thought WebKit code did parenthesis alignment on continued lines
> >   functionName(blah,...
> >                blah, blah, blah);
> 
> That was the specific case that Darin said was not allowed.  See https://bugs.webkit.org/attachment.cgi?id=165498&action=review .

Thanks. I'm just busy revealing how long it has been since I've written any WebKit code.

[Ojan Vafai]

Comment on attachment 165672 [details]
Patch

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

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2072
> +        error(line_number, 'whitespace/indent', 3,
> +              'Weird number of spaces at line-start.  '
> +              'Are you using a 4-space indent?')

Nit: put this string on one line? and maybe put all the arguments on one line? Also, isn't the violating the new style rule?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2081
> +    previous_line_initial_spaces = 0
> +    while previous_line_initial_spaces < len(previous_line) and previous_line[previous_line_initial_spaces] == ' ':
> +        previous_line_initial_spaces += 1

Nit: make a helper function for this?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2084
> +        error(line_number, 'whitespace/indent', 3,
> +              'When wrapping a line, only indent 4 spaces.')

Nit: put this on one line?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:2573
>      # if(match($0, " <<")) complain = 0;
>      # if(match(prev, " +for \\(")) complain = 0;
>      # if(prevodd && match(prevprev, " +for \\(")) complain = 0;

This comment seems way outdated (and related to this patch). Delete them?

[Tony Chang]

Created attachment 165690 [details]
Patch

[Tony Chang]

I'm going to land this tomorrow so I can be around to fix any false positives.

[Luke Macpherson]

IMHO, "The indent size is 4 spaces." in the style guide seems to refer more to new blocks of code than to aligning successive lines in a case like a function call.
http://www.webkit.org/coding/coding-style.html#indentation-wrap-bool-op happens to result in 4-spaces because "if (" is 4 characters long, but I think that's merely a coincidence and the principle is that stuff is aligned to the inside of the opening  "(".

Ironic that the python code for this very patch even uses this form :)

[Tony Chang]

Committed r129690: <http://trac.webkit.org/changeset/129690>