I've been bitten a few times going through the review process with: if( foo ) being flagged as bad style by a reviewer but never flagged by the check-webkit-style script. Since these roundtrips waste everybody's time it would be great if check-webkit-style would flag it on my end (to help newcomers like me learn the rules)
Created attachment 58908 [details] Proposed patch I've updated the unit tests so that all pass.
Comment on attachment 58908 [details] Proposed patch Looks good. Thanks for this patch! Please run the style checker for WebCore/*/*.{cpp,h} and examine if there are no false postives. WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py: + self.assert_lint('while ( foo ) {', 'Should have zero or one spaces inside' I think we still want this test. Maybe this case should produce two errors?
Created attachment 59009 [details] Updated change
(In reply to comment #2) > (From update of attachment 58908 [details]) > Looks good. Thanks for this patch! > > Please run the style checker for WebCore/*/*.{cpp,h} and examine if there are no false postives. > I shouldn't have been so confident :) I noticed that: for(int a = 0; a < 10; ) Got broken with my change, I've fixed that in the revised patch. > WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py: > + self.assert_lint('while ( foo ) {', 'Should have zero or one spaces inside' > I think we still want this test. Maybe this case should produce two errors? This actually will spit out the to messages (one for the space after '(' and one for the space before ')'. I didn't see any other tests that check for two errors, are there anyway?
Created attachment 59010 [details] Proposed patch
Created attachment 59011 [details] The right patch this time, sigh.
Comment on attachment 58908 [details] Proposed patch Cleared Shinichiro Hamaji's review+ from obsolete attachment 58908 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 59011 [details] The right patch this time, sigh. > This actually will spit out the to messages (one for the space after '(' and one for the space before ')'. I didn't see any other tests that check for two errors, are there anyway? I think assert_lint can take a list of errors as the second parameter. Like, self.assert_lint('while ( foo ) {', ['the first error!', 'the second error!'])
Created attachment 59436 [details] Rediff against trunk
Attachment 59436 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py:1183: whitespace before ')' [pep8/E202] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 59436 [details] Rediff against trunk WebKitTools/ChangeLog:13 + 2010-06-16 Sam Magnuson <smagnuson@netflix.com> It's better to put this new changelog entry on the top of this file.
Comment on attachment 59436 [details] Rediff against trunk Also, please fix the style error the review bot found. Otherwise this patch looks good. Thanks!
Ah, you are not a committer yet, right? So, I'll land this patch with fixes I've suggested. You don't need to update the patch again.
Committed r61666: <http://trac.webkit.org/changeset/61666>
http://trac.webkit.org/changeset/61666 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/61666 http://trac.webkit.org/changeset/61667 http://trac.webkit.org/changeset/61668