check-webkit-style: false positive reported for #if macro

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Attachment 71742 [details] patch on Bug 46096, Comment #8 had a false-positive on an #if macro definition:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/assembler/MacroAssemblerARM.cpp:59:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 2 files

I believe the check-webkit-style script though this was a standard if() statement, not an #if macro.

[David Levin]

Created attachment 75440 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 75440 [details]
Patch

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

Otherwise seems fine.

> WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1461
> +            in_preprocessor_directive = match(r'\s*#', line)

Seems you want a ^ in that regexp to lock it to the front of the string.

char* my_invalid_style = "You're #1\n"'; would trigger that regexp even though it shouldn't.

[David Levin]

(In reply to comment #2)
> (From update of attachment 75440 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75440&action=review
> 
> Otherwise seems fine.
> 
> > WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1461
> > +            in_preprocessor_directive = match(r'\s*#', line)
> 
> Seems you want a ^ in that regexp to lock it to the front of the string.
> 
> char* my_invalid_style = "You're #1\n"'; would trigger that regexp even though it shouldn't.
Good call.
fwiw, re.match has an implicit ^ (http://docs.python.org/library/re.html#re.match).
re.search would need the ^

I wish there was only re.search (why have another function just to mean "^" + regex_given?) but match exists, so I guess it makes sense to use it.

[David Levin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 75440 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75440&action=review
> > 
> > Otherwise seems fine.
> > 
> > > WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1461
> > > +            in_preprocessor_directive = match(r'\s*#', line)
> > 
> > Seems you want a ^ in that regexp to lock it to the front of the string.
> > 
> > char* my_invalid_style = "You're #1\n"'; would trigger that regexp even though it shouldn't.
> Good call.
> fwiw, re.match has an implicit ^ (http://docs.python.org/library/re.html#re.match).
> re.search would need the ^
> 
> I wish there was only re.search (why have another function just to mean "^" + regex_given?) but match exists, so I guess it makes sense to use it.

But I'll add test for this though.

[David Levin]

Created attachment 75451 [details]
Patch

[Shinichiro Hamaji]

Comment on attachment 75451 [details]
Patch

looks good

[WebKit Commit Bot]

Comment on attachment 75451 [details]
Patch

Clearing flags on attachment: 75451

Committed r73248: <http://trac.webkit.org/changeset/73248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.