43077 2010-07-27 14:05:02 -0700 check-webkit-style false positive on "new uint32_t" 2010-08-05 00:12:00 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P3 Normal --- 1 senorblanco webkit-unassigned bashi commit-queue hamaji hayato levin oldest_to_newest 256817 0 senorblanco 2010-07-27 14:05:02 -0700 check-webkit-style complains about uint32_t being an invalid identifier when it's used as a template argument. It doesn't complain about it otherwise (e.g., in an array). E.g., see https://bugs.webkit.org/show_bug.cgi?id=43070 WebCore/platform/graphics/skia/PlatformContextSkia.cpp:780: uint32_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] It should probably be allowed in all cases, since it's a useful type and we're not declaring it. 256822 1 levin 2010-07-27 14:11:08 -0700 levin_bot says "It looks like this check was added in http://trac.webkit.org/changeset/51682 which was done by hamaji@chromium.org. Added him to the cc list." 259380 2 63326 bashi 2010-08-03 04:56:28 -0700 Created attachment 63326 Patch 259387 3 63326 hamaji 2010-08-03 05:28:34 -0700 Comment on attachment 63326 Patch So, the real issue isn't T<uint32_t> but new uint32_t ? WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2454 + line = sub(r'new\s*(\(([\w\s:]|::)+\s*[*&]*\s*\))?(?=\W)', '', line) Do we really need |:: ? Maybe '(\([\w\s:]+...\))?' would be OK? WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2454 + line = sub(r'new\s*(\(([\w\s:]|::)+\s*[*&]*\s*\))?(?=\W)', '', line) Why do we need (?=\W) ? 259401 4 bashi 2010-08-03 05:58:51 -0700 (In reply to comment #3) Hamaji-san, > So, the real issue isn't T<uint32_t> but new uint32_t ? I think so. T<uint32_t> isn't the issue at least the case which mentioned on the description. For 'WTF::OwnArrayPtr<uint32_t> buf(new uint32_t[width]);', check-webkit-style could evaluate 'new' as a type of an identifier and uint32_t as an identifier. > WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2454 > + line = sub(r'new\s*(\(([\w\s:]|::)+\s*[*&]*\s*\))?(?=\W)', '', line) > Do we really need |:: ? Maybe '(\([\w\s:]+...\))?' would be OK? I misunderstood the syntax of placement new. |:: isn't needed here. Thank you letting me know that. > WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:2454 > + line = sub(r'new\s*(\(([\w\s:]|::)+\s*[*&]*\s*\))?(?=\W)', '', line) > Why do we need (?=\W) ? Here I also misunderstood the syntax, too. (?=\W) isn't needed here. I'll send revised patch soon. Thanks! 259428 5 63330 bashi 2010-08-03 06:47:11 -0700 Created attachment 63330 Patch V1 259439 6 63330 hamaji 2010-08-03 07:18:16 -0700 Comment on attachment 63330 Patch V1 Looks good. Do you need cq+ as well? If so, you can set both r? and cq? when you put a patch. 259449 7 senorblanco 2010-08-03 07:50:33 -0700 Just FYI, I originally used int32_t* buf = new int32_t[size], which gave no warnings from check-webkit-style. That might be because of the brackets, or some other parsing issue, but switching to OwnArrayPtr<uint32_t> definitely made it start happening. 259488 8 bashi 2010-08-03 08:56:06 -0700 (In reply to comment #7) Hi Stephen, > Just FYI, I originally used int32_t* buf = new int32_t[size], which gave no warnings from check-webkit-style. That might be because of the brackets, or some other parsing issue, but switching to OwnArrayPtr<uint32_t> definitely made it start happening. Switching to OwnArrayPtr<uint32_t> would involve introducing parentheses on the same line and such parentheses are the cause of this false positive. Since parsing declarations correctly is complicate process, check-webkit-style simply assumes that the line would include function arguments when an open parenthesis follows after an identifier. As a result, check-webkit-style could evaluate 'new' as a type and 'uint32_t' as an identifier for the case mentioned in the description of this issue. Another way to fix this issue is treating uint32_t and it's family as special case like 'const_iterator'. What do you think which way is appropriate? I'd like to ask your opinion. Thanks, 259490 9 bashi 2010-08-03 08:59:11 -0700 Hamaji-san, > Looks good. Do you need cq+ as well? If so, you can set both r? and cq? when you put a patch. I'm sorry I forgot to check r? and cq?. Thank you for setting it. 260445 10 63330 commit-queue 2010-08-05 00:11:55 -0700 Comment on attachment 63330 Patch V1 Clearing flags on attachment: 63330 Committed r64718: <http://trac.webkit.org/changeset/64718> 260446 11 commit-queue 2010-08-05 00:12:00 -0700 All reviewed patches have been landed. Closing bug. 63326 2010-08-03 04:56:28 -0700 2010-08-03 06:47:06 -0700 Patch bug-43077-20100803205626.patch text/plain 2599 bashi ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCBlOGRjN2I5OTkxNDY0YjA3NDBiYmNlYWQ1NjI2YmZkZDQ0NTJhOGQ0Li4zZmY4ZDMx ODQ0N2I3MWZhYWYzMzliNjc1OTFlNDFlN2Q5MzEwNmNmIDEwMDY0NAotLS0gYS9XZWJLaXRUb29s cy9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTAtMDgtMDMgIEtlbmljaGkgSXNoaWJhc2hpICA8YmFzaGlAZ29vZ2xlLmNvbT4KKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXhlcyBjaGVjay13 ZWJraXQtc3R5bGUgZmFsc2UgcG9zaXRpdmUgb24gVDx1aW50MzJfdD4uCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MzA3NworCisgICAgICAgICogU2Ny aXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHk6CisgICAgICAgICogU2NyaXB0cy93 ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHBfdW5pdHRlc3QucHk6CisKIDIwMTAtMDgtMDIgIFBh dmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQg YnkgWXVyeSBTZW1pa2hhdHNreS4KZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwLnB5IGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRw eS9zdHlsZS9jaGVja2Vycy9jcHAucHkKaW5kZXggOWU0MjQwYzEwMThhNmYzZjc5MjU4YzZjZTNk MjJlYWMwM2Q2OTg5NS4uZmIzNzA1NzRkOTExMzBiZDFjYWIzOGYxZmU5NzZiZTA0NmQwNmYzOSAx MDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9j cHAucHkKKysrIGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9j cHAucHkKQEAgLTI0NTAsNiArMjQ1MCw4IEBAIGRlZiBjaGVja19pZGVudGlmaWVyX25hbWVfaW5f ZGVjbGFyYXRpb24oZmlsZW5hbWUsIGxpbmVfbnVtYmVyLCBsaW5lLCBlcnJvcik6CiAgICAgIyBD b252ZXJ0IHVuc2lnbmVkL3NpZ25lZCB0eXBlcyB0byBzaW1wbGUgdHlwZXMsIHRvby4KICAgICBs aW5lID0gc3ViKHInKHVuc2lnbmVkfHNpZ25lZCkgKD89Y2hhcnxzaG9ydHxpbnR8bG9uZyknLCAn JywgbGluZSkKICAgICBsaW5lID0gc3ViKHInXGIoaW5saW5lfHVzaW5nfHN0YXRpY3xjb25zdHx2 b2xhdGlsZXxhdXRvfHJlZ2lzdGVyfGV4dGVybnx0eXBlZGVmfHJlc3RyaWN0fHN0cnVjdHxjbGFz c3x2aXJ0dWFsKSg/PVxXKScsICcnLCBsaW5lKQorICAgICMgQ29udmVydCAibmV3IiwgIm5ldyAo ZXhwcikiIHRvIHNpbXBsZSB0eXBlcywgdG9vLgorICAgIGxpbmUgPSBzdWIociduZXdccyooXCgo W1x3XHM6XXw6OikrXHMqWyomXSpccypcKSk/KD89XFcpJywgJycsIGxpbmUpCiAKICAgICAjIFJl bW92ZSBhbGwgdGVtcGxhdGUgcGFyYW1ldGVycyBieSByZW1vdmluZyBtYXRjaGluZyA8IGFuZCA+ LgogICAgICMgTG9vcCB1bnRpbCBubyB0ZW1wbGF0ZXMgYXJlIHJlbW92ZWQgdG8gcmVtb3ZlIG5l c3RlZCB0ZW1wbGF0ZXMuCmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5 L3N0eWxlL2NoZWNrZXJzL2NwcF91bml0dGVzdC5weSBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CmluZGV4IGM5MjdkYjZmYjk3ZDUz OGI1ZGRjOGQzYmMzMzU3NDFiZjIwY2E4NjYuLjE2YjFhM2MwNGRkZmNjZmFmZmM0ZDhiMTdjZDM1 NDFjMGYwODEyZDAgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5 bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CkBAIC0zNzE1LDYgKzM3MTUsMTIg QEAgY2xhc3MgV2ViS2l0U3R5bGVUZXN0KENwcFN0eWxlVGVzdEJhc2UpOgogICAgICAgICBzZWxm LmFzc2VydF9saW50KCd1bnNpZ25lZCBfZmlsbFJ1bGUgOiAxOycsCiAgICAgICAgICAgICAgICAg ICAgICAgICAgJ19maWxsUnVsZScgKyBuYW1lX3VuZGVyc2NvcmVfZXJyb3JfbWVzc2FnZSkKIAor ICAgICAgICAjIG5ldyBvcGVyYXRvcnMgaW4gaW5pdGlhbGl6YXRpb24uCisgICAgICAgIHNlbGYu YXNzZXJ0X2xpbnQoJ093blB0cjx1aW50MzJfdD4gdmFyaWFibGUobmV3IHVpbnQzMl90KTsnLCAn JykKKyAgICAgICAgc2VsZi5hc3NlcnRfbGludCgnT3duUHRyPHVpbnQzMl90PiB2YXJpYWJsZShu ZXcgKGV4cHIpIHVpbnQzMl90KTsnLCAnJykKKyAgICAgICAgc2VsZi5hc3NlcnRfbGludCgnT3du UHRyPHVpbnQzMl90PiB1bmRlcl9zY29yZShuZXcgdWludDMyX3QpOycsCisgICAgICAgICAgICAg ICAgICAgICAgICAgJ3VuZGVyX3Njb3JlJyArIG5hbWVfdW5kZXJzY29yZV9lcnJvcl9tZXNzYWdl KQorCiAKICAgICBkZWYgdGVzdF9jb21tZW50cyhzZWxmKToKICAgICAgICAgIyBBIGNvbW1lbnQg YXQgdGhlIGJlZ2lubmluZyBvZiBhIGxpbmUgaXMgb2suCg== 63330 2010-08-03 06:47:11 -0700 2010-08-05 00:11:55 -0700 Patch V1 bug-43077-20100803224709.patch text/plain 2545 bashi ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA3ZWUwZDQ0ZGI0MzliMmI4MzZkMmIyNDBmOTk2Y2ZiNDYzYjA1MGNiLi42MWUxNGRj MTMwMGFlNGIwOGQyNmJkNWFlMTJmOThhNmQ3ZTg3NTBhIDEwMDY0NAotLS0gYS9XZWJLaXRUb29s cy9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTAtMDgtMDMgIEtlbmljaGkgSXNoaWJhc2hpICA8YmFzaGlAZ29vZ2xlLmNvbT4KKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXhlcyBjaGVjay13 ZWJraXQtc3R5bGUgZmFsc2UgcG9zaXRpdmUgb24gIm5ldyB1aW50MzJfdC4iCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MzA3NworCisgICAgICAgICog U2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHAucHk6CisgICAgICAgICogU2NyaXB0 cy93ZWJraXRweS9zdHlsZS9jaGVja2Vycy9jcHBfdW5pdHRlc3QucHk6CisKIDIwMTAtMDgtMDMg IEpvY2hlbiBFaXNpbmdlciAgPGpvY2hlbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgVW5yZXZp ZXdlZC4gQWRkaW5nIG15c2VsZiBhcyBjb21taXR0ZXIuCmRpZmYgLS1naXQgYS9XZWJLaXRUb29s cy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXJzL2NwcC5weSBiL1dlYktpdFRvb2xzL1Nj cmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwLnB5CmluZGV4IDllNDI0MGMxMDE4YTZm M2Y3OTI1OGM2Y2UzZDIyZWFjMDNkNjk4OTUuLjYyZjQwZWFiMzdmYjQ5Y2ZjYmIxYmE5MmQ0NGJk MmI0MWMyYTI4YjggMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5 bGUvY2hlY2tlcnMvY3BwLnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5 bGUvY2hlY2tlcnMvY3BwLnB5CkBAIC0yNDUxLDYgKzI0NTEsOSBAQCBkZWYgY2hlY2tfaWRlbnRp Zmllcl9uYW1lX2luX2RlY2xhcmF0aW9uKGZpbGVuYW1lLCBsaW5lX251bWJlciwgbGluZSwgZXJy b3IpOgogICAgIGxpbmUgPSBzdWIocicodW5zaWduZWR8c2lnbmVkKSAoPz1jaGFyfHNob3J0fGlu dHxsb25nKScsICcnLCBsaW5lKQogICAgIGxpbmUgPSBzdWIocidcYihpbmxpbmV8dXNpbmd8c3Rh dGljfGNvbnN0fHZvbGF0aWxlfGF1dG98cmVnaXN0ZXJ8ZXh0ZXJufHR5cGVkZWZ8cmVzdHJpY3R8 c3RydWN0fGNsYXNzfHZpcnR1YWwpKD89XFcpJywgJycsIGxpbmUpCiAKKyAgICAjIFJlbW92ZSAi bmV3IiBhbmQgIm5ldyAoZXhwcikiIHRvIHNpbXBsaWZ5LCB0b28uCisgICAgbGluZSA9IHN1Yihy J25ld1xzKihcKFteKV0qXCkpPycsICcnLCBsaW5lKQorCiAgICAgIyBSZW1vdmUgYWxsIHRlbXBs YXRlIHBhcmFtZXRlcnMgYnkgcmVtb3ZpbmcgbWF0Y2hpbmcgPCBhbmQgPi4KICAgICAjIExvb3Ag dW50aWwgbm8gdGVtcGxhdGVzIGFyZSByZW1vdmVkIHRvIHJlbW92ZSBuZXN0ZWQgdGVtcGxhdGVz LgogICAgIHdoaWxlIFRydWU6CmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtp dHB5L3N0eWxlL2NoZWNrZXJzL2NwcF91bml0dGVzdC5weSBiL1dlYktpdFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CmluZGV4IGM5MjdkYjZmYjk3 ZDUzOGI1ZGRjOGQzYmMzMzU3NDFiZjIwY2E4NjYuLjE2YjFhM2MwNGRkZmNjZmFmZmM0ZDhiMTdj ZDM1NDFjMGYwODEyZDAgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkv c3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvc3R5bGUvY2hlY2tlcnMvY3BwX3VuaXR0ZXN0LnB5CkBAIC0zNzE1LDYgKzM3MTUs MTIgQEAgY2xhc3MgV2ViS2l0U3R5bGVUZXN0KENwcFN0eWxlVGVzdEJhc2UpOgogICAgICAgICBz ZWxmLmFzc2VydF9saW50KCd1bnNpZ25lZCBfZmlsbFJ1bGUgOiAxOycsCiAgICAgICAgICAgICAg ICAgICAgICAgICAgJ19maWxsUnVsZScgKyBuYW1lX3VuZGVyc2NvcmVfZXJyb3JfbWVzc2FnZSkK IAorICAgICAgICAjIG5ldyBvcGVyYXRvcnMgaW4gaW5pdGlhbGl6YXRpb24uCisgICAgICAgIHNl bGYuYXNzZXJ0X2xpbnQoJ093blB0cjx1aW50MzJfdD4gdmFyaWFibGUobmV3IHVpbnQzMl90KTsn LCAnJykKKyAgICAgICAgc2VsZi5hc3NlcnRfbGludCgnT3duUHRyPHVpbnQzMl90PiB2YXJpYWJs ZShuZXcgKGV4cHIpIHVpbnQzMl90KTsnLCAnJykKKyAgICAgICAgc2VsZi5hc3NlcnRfbGludCgn T3duUHRyPHVpbnQzMl90PiB1bmRlcl9zY29yZShuZXcgdWludDMyX3QpOycsCisgICAgICAgICAg ICAgICAgICAgICAgICAgJ3VuZGVyX3Njb3JlJyArIG5hbWVfdW5kZXJzY29yZV9lcnJvcl9tZXNz YWdlKQorCiAKICAgICBkZWYgdGVzdF9jb21tZW50cyhzZWxmKToKICAgICAgICAgIyBBIGNvbW1l bnQgYXQgdGhlIGJlZ2lubmluZyBvZiBhIGxpbmUgaXMgb2suCg==