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.
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."
Created attachment 63326 [details] Patch
Comment on attachment 63326 [details] 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) ?
(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!
Created attachment 63330 [details] Patch V1
Comment on attachment 63330 [details] Patch V1 Looks good. Do you need cq+ as well? If so, you can set both r? and cq? when you put a patch.
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.
(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,
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.
Comment on attachment 63330 [details] Patch V1 Clearing flags on attachment: 63330 Committed r64718: <http://trac.webkit.org/changeset/64718>
All reviewed patches have been landed. Closing bug.