Bug 40723 – Fix paren matching rules for switch/if/while in check-webkit-style script

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Magnuson]

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)

[Sam Magnuson]

Created attachment 58908 [details]
Proposed patch

I've updated the unit tests so that all pass.

[Shinichiro Hamaji]

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?

[Sam Magnuson]

Created attachment 59009 [details]
Updated change

[Sam Magnuson]

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

[Sam Magnuson]

Created attachment 59010 [details]
Proposed patch

[Sam Magnuson]

Created attachment 59011 [details]
The right patch this time, sigh.

[Eric Seidel (no email)]

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.

[Shinichiro Hamaji]

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!'])

[Sam Magnuson]

Created attachment 59436 [details]
Rediff against trunk

[WebKit Review Bot]

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.

[Shinichiro Hamaji]

Comment on attachment 59436 [details]
Rediff against trunk

WebKitTools/ChangeLog:13
 +  2010-06-16  Sam Magnuson  <[e-mail]>
It's better to put this new changelog entry on the top of this file.

[Shinichiro Hamaji]

Comment on attachment 59436 [details]
Rediff against trunk

Also, please fix the style error the review bot found. Otherwise this patch looks good. Thanks!

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

Committed r61666: <http://trac.webkit.org/changeset/61666>

[WebKit Review Bot]

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