62097 2011-06-04 13:53:29 -0700 regression(87771): ternary operator incorrectly identified as bitfield 2011-06-06 12:36:24 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 alex yong.li.webkit eric levin webkit.review.bot yong.li.webkit oldest_to_newest 415531 0 alex 2011-06-04 13:53:29 -0700 After the patch for bug 54807 was applied, the style check now incorrectly identifies code like: int heightDiff = m_scripts ? (m_scripts->offsetHeight() - maxHeight) / 2 : 0; as a bitfield. I had to change the code to: int heightDiff = m_scripts ? (m_scripts->offsetHeight() - maxHeight) / 2 : 0; to get it to pass the style check. 415544 1 eric 2011-06-04 14:29:23 -0700 I remember seeing that change go by. Guess it needs some tweaking. 415545 2 eric 2011-06-04 14:30:24 -0700 I wouldn't bother changing your code to make it pass the style check. the style checker just needs to be fixed. 415843 3 yong.li.webkit 2011-06-06 08:13:52 -0700 (In reply to comment #2) > I wouldn't bother changing your code to make it pass the style check. the style checker just needs to be fixed. I should have filtered out the ?: case... 415845 4 yong.li.webkit 2011-06-06 08:17:35 -0700 I'm working on this now. 415855 5 96096 yong.li.webkit 2011-06-06 09:09:13 -0700 Created attachment 96096 the patch 415885 6 96096 eric 2011-06-06 10:37:05 -0700 Comment on attachment 96096 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=96096&action=review > Tools/Scripts/webkitpy/style/checkers/cpp.py:2925 > + matched = re.match(r'\s*((const|mutable)\s+)?(char|(short(\s+int)?)|int|long(\s+(long|int))?)\s+[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*\d+\s*;', line) seems you meant + not *, or? Unfortunate that we don't have a nice constant for "identifier" since that long regexp you added may be used in other places? 415931 7 96096 yong.li.webkit 2011-06-06 11:55:56 -0700 Comment on attachment 96096 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=96096&action=review >> Tools/Scripts/webkitpy/style/checkers/cpp.py:2925 >> + matched = re.match(r'\s*((const|mutable)\s+)?(char|(short(\s+int)?)|int|long(\s+(long|int))?)\s+[a-zA-Z_][a-zA-Z0-9_]*\s*:\s*\d+\s*;', line) > > seems you meant + not *, or? > > Unfortunate that we don't have a nice constant for "identifier" since that long regexp you added may be used in other places? The first character cannot be digit, so there must be one [a-zA-Z_]. Then the left can be 0-multiple [a-zA-Z0-9_]. But you are right, [a-zA-Z0-9_]+ should be enough in this case, and it is used at line 2791. I'm not sure if we prefer strict match or loose (for parsing speed?). Seems every places in the file are doing their own jobs. I tend to leave the refine to the future patches :-P 415949 8 96096 webkit.review.bot 2011-06-06 12:36:20 -0700 Comment on attachment 96096 the patch Clearing flags on attachment: 96096 Committed r88179: <http://trac.webkit.org/changeset/88179> 415950 9 webkit.review.bot 2011-06-06 12:36:24 -0700 All reviewed patches have been landed. Closing bug. 96096 2011-06-06 09:09:13 -0700 2011-06-06 12:36:19 -0700 the patch 62097.patch text/plain 2164 yong.li.webkit ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCA0OTQ5 YmU1Li4yZjRmMDg0IDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTEtMDYtMDYgIFlvbmcgTGkgIDx5b2xpQHJpbS5j b20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYyMDk3CisgICAgICAgIEZpeCB0 aGUgcHJvYmxlbSB0aGF0IGludGVncmFsIGJpdGZpZWxkIGNoZWNrIGluY29ycmVjdGx5IGJhcmtl ZCBhdCA/OiBleHByZXNzaW9ucy4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUv Y2hlY2tlcnMvY3BwLnB5OgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tl cnMvY3BwX3VuaXR0ZXN0LnB5OgorCiAyMDExLTAyLTIyICBZb25nIExpICA8eW9saUByaW0uY29t PgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVyaWMgU2VpZGVsLgpkaWZmIC0tZ2l0IGEvVG9vbHMv U2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHkgYi9Ub29scy9TY3JpcHRzL3dl YmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weQppbmRleCA3NTQyNTVhLi5jYTVkMWQ5IDEwMDY0 NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weQorKysg Yi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weQpAQCAtMjkyMiw3 ICsyOTIyLDcgQEAgZGVmIGNoZWNrX2xhbmd1YWdlKGZpbGVuYW1lLCBjbGVhbl9saW5lcywgbGlu ZV9udW1iZXIsIGZpbGVfZXh0ZW5zaW9uLCBpbmNsdWRlX3MKICAgICAjIENoZWNrIGZvciBwbGFp biBiaXRmaWVsZHMgZGVjbGFyZWQgd2l0aG91dCBlaXRoZXIgInNpbmdlZCIgb3IgInVuc2lnbmVk Ii4KICAgICAjIE1vc3QgY29tcGlsZXJzIHRyZWF0IHN1Y2ggYml0ZmllbGRzIGFzIHNpZ25lZCwg YnV0IHRoZXJlIGFyZSBzdGlsbCBjb21waWxlcnMgbGlrZQogICAgICMgUlZDVCA0LjAgdGhhdCB1 c2UgdW5zaWduZWQgYnkgZGVmYXVsdC4KLSAgICBtYXRjaGVkID0gcmUubWF0Y2gocidccyooKGNv bnN0fG11dGFibGUpXHMrKT8oY2hhcnwoc2hvcnQoXHMraW50KT8pfGludHxsb25nKFxzKyhsb25n fGludCkpPylccysuK1xzKjpccypcZCtccyo7JywgbGluZSkKKyAgICBtYXRjaGVkID0gcmUubWF0 Y2gocidccyooKGNvbnN0fG11dGFibGUpXHMrKT8oY2hhcnwoc2hvcnQoXHMraW50KT8pfGludHxs b25nKFxzKyhsb25nfGludCkpPylccytbYS16QS1aX11bYS16QS1aMC05X10qXHMqOlxzKlxkK1xz KjsnLCBsaW5lKQogICAgIGlmIG1hdGNoZWQ6CiAgICAgICAgIGVycm9yKGxpbmVfbnVtYmVyLCAn cnVudGltZS9iaXRmaWVsZHMnLCA1LAogICAgICAgICAgICAgICAnUGxlYXNlIGRlY2xhcmUgaW50 ZWdyYWwgdHlwZSBiaXRmaWVsZHMgd2l0aCBlaXRoZXIgc2lnbmVkIG9yIHVuc2lnbmVkLicpCmRp ZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcF91bml0 dGVzdC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0 ZXN0LnB5CmluZGV4IDkyZTA0OTUuLjFiZDhhMGUgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMv d2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL1Njcmlw dHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CkBAIC0yMzgxLDYgKzIz ODEsNyBAQCBjbGFzcyBDcHBTdHlsZVRlc3QoQ3BwU3R5bGVUZXN0QmFzZSk6CiAgICAgICAgIHNl bGYuYXNzZXJ0X2xpbnQoJ211dGFibGUgc2hvcnQgYSA6IDE0OycsIGVycm1zZykKICAgICAgICAg c2VsZi5hc3NlcnRfbGludCgnY29uc3QgY2hhciBhIDogNjsnLCBlcnJtc2cpCiAgICAgICAgIHNl bGYuYXNzZXJ0X2xpbnQoJ2xvbmcgaW50IGEgOiAzMDsnLCBlcnJtc2cpCisgICAgICAgIHNlbGYu YXNzZXJ0X2xpbnQoJ2ludCBhID0gMSA/IDAgOiAzMDsnLCAnJykKIAogY2xhc3MgQ2xlYW5zZWRM aW5lc1Rlc3QodW5pdHRlc3QuVGVzdENhc2UpOgogICAgIGRlZiB0ZXN0X2luaXQoc2VsZik6Cg==