Summary: | C++ style checker should warn when the indentation is wrong | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||
Component: | New Bugs | Assignee: | Tony Chang <tony> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, darin, dpranke, levin, macpherson, ojan, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tony Chang
2012-09-25 14:11:52 PDT
Created attachment 165672 [details]
Patch
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. 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); 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 . (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. 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? Created attachment 165690 [details]
Patch
I'm going to land this tomorrow so I can be around to fix any false positives. 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 :) Committed r129690: <http://trac.webkit.org/changeset/129690> |