RESOLVED FIXED 51695
check-webkit-style treated some macros with parentheses after #elif as function calls 
https://bugs.webkit.org/show_bug.cgi?id=51695
Summary check-webkit-style treated some macros with parentheses after #elif as functi...
Koan-Sin Tan
Reported 2010-12-28 19:10:06 PST
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')
Attachments
proposed patch (1.54 KB, patch)
2011-01-04 17:14 PST, Koan-Sin Tan 		levin: review-
DetailsFormatted DiffDiff
proposed patch (2.44 KB, patch)
2011-01-04 18:31 PST, Koan-Sin Tan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Koan-Sin Tan
Comment 1 2010-12-29 05:03:00 PST
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
Comment 2 2010-12-31 09:05:58 PST
(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
Comment 3 2011-01-02 16:51:26 PST
(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
Comment 4 2011-01-02 19:33:20 PST
(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
Comment 5 2011-01-04 16:39:22 PST
(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
Comment 6 2011-01-04 17:14:51 PST
Created attachment 77947 [details] proposed patch proposed patch as suggested by David Levin
David Levin
Comment 7 2011-01-04 17:26:20 PST
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
Comment 8 2011-01-04 18:31:31 PST
Created attachment 77959 [details] proposed patch add two unit test cases
WebKit Commit Bot
Comment 9 2011-01-04 23:33:18 PST
Comment on attachment 77959 [details] proposed patch Clearing flags on attachment: 77959 Committed r75050: <http://trac.webkit.org/changeset/75050>
WebKit Commit Bot
Comment 10 2011-01-04 23:33:24 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.