51695 2010-12-28 19:10:06 -0800 check-webkit-style treated some macros with parentheses after #elif as function calls 2011-01-04 23:33:24 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 koansin.tan webkit-unassigned commit-queue levin mrobinson mrowe oldest_to_newest 327468 0 koansin.tan 2010-12-28 19:10:06 -0800 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') 327538 1 koansin.tan 2010-12-29 05:03:00 -0800 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. 328180 2 levin 2010-12-31 09:05:58 -0800 (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*.) 328445 3 koansin.tan 2011-01-02 16:51:26 -0800 (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 328469 4 levin 2011-01-02 19:33:20 -0800 (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. 329297 5 koansin.tan 2011-01-04 16:39:22 -0800 (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 329308 6 77947 koansin.tan 2011-01-04 17:14:51 -0800 Created attachment 77947 proposed patch proposed patch as suggested by David Levin 329319 7 77947 levin 2011-01-04 17:26:20 -0800 Comment on attachment 77947 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. 329368 8 77959 koansin.tan 2011-01-04 18:31:31 -0800 Created attachment 77959 proposed patch add two unit test cases 329450 9 77959 commit-queue 2011-01-04 23:33:18 -0800 Comment on attachment 77959 proposed patch Clearing flags on attachment: 77959 Committed r75050: <http://trac.webkit.org/changeset/75050> 329451 10 commit-queue 2011-01-04 23:33:24 -0800 All reviewed patches have been landed. Closing bug. 77947 2011-01-04 17:14:51 -0800 2011-01-04 18:31:31 -0800 proposed patch elif.diff text/plain 1579 koansin.tan SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDc1MDI4KQorKysgVG9vbHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTYgQEAKKzIwMTEtMDEtMDQgIEtvYW4tU2luIFRhbiAgPGtvYW5zaW4udGFuQGdtYWlsLmNv bT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBjaGVj ay13ZWJraXQtc3R5bGUgdHJlYXRlZCBzb21lIG1hY3JvcyB3aXRoIHBhcmVudGhlc2VzIGFmdGVy ICNlbGlmIGFzIGZ1bmN0aW9uIGNhbGxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD01MTY5NQorCisgICAgICAgIEluZ29yZSBmdW5jdGlvbiBjYWxsIHNw YWNlIGNoZWNraW5nIGluIGFueSBwcmVwcm9jZXNzb3IgZGlyZWN0aXZlcworICAgICAgICAodGhp bmdzIHN0YXJ0aW5nIHdpdGggIykuIENoYW5nZSBzZWFyY2goKSB0byBtYXRjaCgpIGJlY2F1c2Ug CisgICAgICAgIHByZXByb2Nlc3NvciBkaXJlY3RpdmVzIGFyZSBzdXBwb3NlZCB0byBiZSBpbiB0 aGUgYmVnaW5uaW5nIG9mIGxpbmVzLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9zdHls ZS9jaGVja2Vycy9jcHAucHk6CisKIDIwMTEtMDEtMDMgIE1hcnRpbiBSb2JpbnNvbiAgPG1yb2Jp bnNvbkBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgpJbmRl eDogVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkJKHJldmlz aW9uIDc1MDI4KQorKysgVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAu cHkJKHdvcmtpbmcgY29weSkKQEAgLTEzMzgsNyArMTMzOCw3IEBAIGRlZiBjaGVja19zcGFjaW5n X2Zvcl9mdW5jdGlvbl9jYWxsKGxpbmUKICAgICAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAn d2hpdGVzcGFjZS9wYXJlbnMnLCAyLAogICAgICAgICAgICAgICAgICAgJ0V4dHJhIHNwYWNlIGFm dGVyICgnKQogICAgICAgICBpZiAoc2VhcmNoKHInXHdccytcKCcsIGZ1bmN0aW9uX2NhbGwpCi0g ICAgICAgICAgICBhbmQgbm90IHNlYXJjaChyJyNccypkZWZpbmV8dHlwZWRlZicsIGZ1bmN0aW9u X2NhbGwpKToKKyAgICAgICAgICAgIGFuZCBub3QgbWF0Y2gocidccyooI3x0eXBlZGVmKScsIGZ1 bmN0aW9uX2NhbGwpKToKICAgICAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAnd2hpdGVzcGFj ZS9wYXJlbnMnLCA0LAogICAgICAgICAgICAgICAgICAgJ0V4dHJhIHNwYWNlIGJlZm9yZSAoIGlu IGZ1bmN0aW9uIGNhbGwnKQogICAgICAgICAjIElmIHRoZSApIGlzIGZvbGxvd2VkIG9ubHkgYnkg YSBuZXdsaW5lIG9yIGEgeyArIG5ld2xpbmUsIGFzc3VtZSBpdCdzCg== 77959 2011-01-04 18:31:31 -0800 2011-01-04 23:33:18 -0800 proposed patch elif.diff text/plain 2496 koansin.tan SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDc1MDM5KQorKysgVG9vbHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTcgQEAKKzIwMTEtMDEtMDQgIEtvYW4tU2luIFRhbiAgPGtvYW5zaW4udGFuQGdtYWlsLmNv bT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBjaGVj ay13ZWJraXQtc3R5bGUgdHJlYXRlZCBzb21lIG1hY3JvcyB3aXRoIHBhcmVudGhlc2VzIGFmdGVy ICNlbGlmIGFzIGZ1bmN0aW9uIGNhbGxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD01MTY5NQorCisgICAgICAgIEluZ29yZSBmdW5jdGlvbiBjYWxsIHNw YWNlIGNoZWNraW5nIGluIGFueSBwcmVwcm9jZXNzb3IgZGlyZWN0aXZlcworICAgICAgICAodGhp bmdzIHN0YXJ0aW5nIHdpdGggIykuIENoYW5nZSBzZWFyY2goKSB0byBtYXRjaCgpIGJlY2F1c2Ug CisgICAgICAgIHByZXByb2Nlc3NvciBkaXJlY3RpdmVzIGFyZSBzdXBwb3NlZCB0byBiZSBpbiB0 aGUgYmVnaW5uaW5nIG9mIGxpbmVzLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9zdHls ZS9jaGVja2Vycy9jcHAucHk6CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVj a2Vycy9jcHBfdW5pdHRlc3QucHk6IG1ha2Ugc3VyZSB0aGVyZSBpcyBubyBmYWxzZSBwb3NpdGl2 ZXMgZm9yICNlbGlmIGNhc2VzCisKIDIwMTEtMDEtMDQgIFPDuHJlbiBHamVzc2UgIDxzZ2plc3Nl QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBUb255IENoYW5nLgpJbmRleDog VG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkJKHJldmlzaW9u IDc1MDM5KQorKysgVG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkJ KHdvcmtpbmcgY29weSkKQEAgLTEzMzgsNyArMTMzOCw3IEBAIGRlZiBjaGVja19zcGFjaW5nX2Zv cl9mdW5jdGlvbl9jYWxsKGxpbmUKICAgICAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAnd2hp dGVzcGFjZS9wYXJlbnMnLCAyLAogICAgICAgICAgICAgICAgICAgJ0V4dHJhIHNwYWNlIGFmdGVy ICgnKQogICAgICAgICBpZiAoc2VhcmNoKHInXHdccytcKCcsIGZ1bmN0aW9uX2NhbGwpCi0gICAg ICAgICAgICBhbmQgbm90IHNlYXJjaChyJyNccypkZWZpbmV8dHlwZWRlZicsIGZ1bmN0aW9uX2Nh bGwpKToKKyAgICAgICAgICAgIGFuZCBub3QgbWF0Y2gocidccyooI3x0eXBlZGVmKScsIGZ1bmN0 aW9uX2NhbGwpKToKICAgICAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAnd2hpdGVzcGFjZS9w YXJlbnMnLCA0LAogICAgICAgICAgICAgICAgICAgJ0V4dHJhIHNwYWNlIGJlZm9yZSAoIGluIGZ1 bmN0aW9uIGNhbGwnKQogICAgICAgICAjIElmIHRoZSApIGlzIGZvbGxvd2VkIG9ubHkgYnkgYSBu ZXdsaW5lIG9yIGEgeyArIG5ld2xpbmUsIGFzc3VtZSBpdCdzCkluZGV4OiBUb29scy9TY3JpcHRz L3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcF91bml0dGVzdC5weQo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBU b29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcF91bml0dGVzdC5weQkocmV2 aXNpb24gNzUwMzkpCisrKyBUb29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2Nw cF91bml0dGVzdC5weQkod29ya2luZyBjb3B5KQpAQCAtMTQzNSw2ICsxNDM1LDggQEAgY2xhc3Mg Q3BwU3R5bGVUZXN0KENwcFN0eWxlVGVzdEJhc2UpOgogICAgICAgICBzZWxmLmFzc2VydF9saW50 KCcoKGErYikpJywgJycpCiAgICAgICAgIHNlbGYuYXNzZXJ0X2xpbnQoJ2ZvbyAoZm9vKScsICdF eHRyYSBzcGFjZSBiZWZvcmUgKCBpbiBmdW5jdGlvbiBjYWxsJwogICAgICAgICAgICAgICAgICAg ICAgICAgICcgIFt3aGl0ZXNwYWNlL3BhcmVuc10gWzRdJykKKyAgICAgICAgc2VsZi5hc3NlcnRf bGludCgnI2VsaWYgKGZvbyhiYXIpKScsICcnKQorICAgICAgICBzZWxmLmFzc2VydF9saW50KCcj ZWxpZiAoZm9vKGJhcikgJiYgZm9vKGJheikpJywgJycpCiAgICAgICAgIHNlbGYuYXNzZXJ0X2xp bnQoJ3R5cGVkZWYgZm9vICgqZm9vKShmb28pJywgJycpCiAgICAgICAgIHNlbGYuYXNzZXJ0X2xp bnQoJ3R5cGVkZWYgZm9vICgqZm9vMTJiYXJfKShmb28pJywgJycpCiAgICAgICAgIHNlbGYuYXNz ZXJ0X2xpbnQoJ3R5cGVkZWYgZm9vIChGb286OipiYXIpKGZvbyknLCAnJykK