Summary: | check-webkit-style false positive on "new uint32_t" | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bashi, commit-queue, hamaji, hayato, levin | ||||||
Priority: | P3 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows 7 | ||||||||
Attachments: |
|
Description
Stephen White
2010-07-27 14:05:02 PDT
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. |