Bug 51695 – check-webkit-style treated some macros with parentheses after #elif as function calls

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Koan-Sin Tan]

I ran into this problem when I tried
    #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF))
The message is like:
   WebCore/platform/UUID.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]

I tried a bit and found that simply
   #elif (OS(LINUX))
will have the problem. As David Levin pointed out in comments 26 of bug 50867,  the issue is in the regex below from Tools/Scripts/webkitpy/style/checkers/cpp.py 

1340            if (search(r'\w\s+\(', function_call)
1341                and not search(r'#\s*define|typedef', function_call)):
1342                error(line_number, 'whitespace/parens', 4,
1343                      'Extra space before ( in function call')

[Koan-Sin Tan]

a stupid hack like this will avoid the issue, but this doesn't really look right


Index: Tools/Scripts/webkitpy/style/checkers/cpp.py
===================================================================
--- Tools/Scripts/webkitpy/style/checkers/cpp.py	(revision 74733)
+++ Tools/Scripts/webkitpy/style/checkers/cpp.py	(working copy)
@@ -1327,6 +1327,8 @@
     # they'll never need to wrap.
     if (  # Ignore control structures.
         not search(r'\b(if|for|foreach|while|switch|return|new|delete)\b', function_call)
+        # Ignore (()) right after '#elif '.
+        and not search(r'\A#elif', function_call)
         # Ignore pointers/references to functions.
         and not search(r' \([^)]+\)\([^)]*(\)|,$)', function_call)
         # Ignore pointers/references to arrays.

[David Levin]

(In reply to comment #1)
> a stupid hack like this will avoid the issue, but this doesn't really look right
> 
> 
> Index: Tools/Scripts/webkitpy/style/checkers/cpp.py
> ===================================================================
> --- Tools/Scripts/webkitpy/style/checkers/cpp.py    (revision 74733)
> +++ Tools/Scripts/webkitpy/style/checkers/cpp.py    (working copy)
> @@ -1327,6 +1327,8 @@
>      # they'll never need to wrap.
>      if (  # Ignore control structures.
>          not search(r'\b(if|for|foreach|while|switch|return|new|delete)\b', function_call)
> +        # Ignore (()) right after '#elif '.
> +        and not search(r'\A#elif', function_call)
>          # Ignore pointers/references to functions.
>          and not search(r' \([^)]+\)\([^)]*(\)|,$)', function_call)
>          # Ignore pointers/references to arrays.

I  think you could simply change line 1341 to
1341                and not match(r'\s*(#|typedef)', function_call)):

Which will allow match any preprocessor directive or a typedef. (I switch from search to match because match only matches from the beginning of the line, which is why I added the \s*.)

[Koan-Sin Tan]

(In reply to comment #2)
> > Index: Tools/Scripts/webkitpy/style/checkers/cpp.py
> > > 
> I  think you could simply change line 1341 to
> 1341                and not match(r'\s*(#|typedef)', function_call)):
> 
> Which will allow match any preprocessor directive or a typedef. (I switch from search to match because match only matches from the beginning of the line, which is why I added the \s*.)

it looks better than my stupid hack. Maybe I should mention that
   #if (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF))
would not run into #elif problem, so I think maybe there is a way to avoid calling check_spacing_for_function_call

[David Levin]

(In reply to comment #3)
> (In reply to comment #2)
> > > Index: Tools/Scripts/webkitpy/style/checkers/cpp.py
> > > > 
> > I  think you could simply change line 1341 to
> > 1341                and not match(r'\s*(#|typedef)', function_call)):
> > 
> > Which will allow match any preprocessor directive or a typedef. (I switch from search to match because match only matches from the beginning of the line, which is why I added the \s*.)
> 
> it looks better than my stupid hack. Maybe I should mention that
>    #if (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF))
> would not run into #elif problem, so I think maybe there is a way to avoid calling check_spacing_for_function_call

The reason that "#if" doesn't trigger the warning is just plain luck. The line
         not search(r'\b(if|for|foreach|while|switch|return|new|delete)\b', function_call)
excludes it but it is thereto detect the "if" statement. It just also happens to work for the #if preprocessor directive.

[Koan-Sin Tan]

(In reply to comment #4)
>
> The reason that "#if" doesn't trigger the warning is just plain luck. The line
>          not search(r'\b(if|for|foreach|while|switch|return|new|delete)\b', function_call)
> excludes it but it is thereto detect the "if" statement. It just also happens to work for the #if preprocessor directive.

Ah ha. Got it. So I'll submit a patch based on your comment

[Koan-Sin Tan]

Created attachment 77947 [details]
proposed patch

proposed patch as suggested by David Levin

[David Levin]

Comment on attachment 77947 [details]
proposed patch

Really close!

It looks good but it needs a simple test to verify it. See cpp_unittest.py. Look for the error "Extra space before (" to see where similar tests are and add one which verifies that you don't get the error for the false positive (#elif if I remember correctly).

Run the test using "test-webkitpy" to verify that your test passes.

[Koan-Sin Tan]

Created attachment 77959 [details]
proposed patch

add two unit test cases

[WebKit Commit Bot]

Comment on attachment 77959 [details]
proposed patch

Clearing flags on attachment: 77959

Committed r75050: <http://trac.webkit.org/changeset/75050>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.