check-webkit-style false positive on "new uint32_t"

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Stephen White]

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.

[David Levin]

levin_bot says "It looks like this check was added in http://trac.webkit.org/changeset/51682 which was done by [e-mail]. Added him to the cc list."

[Kenichi Ishibashi]

Created attachment 63326 [details]
Patch

[Shinichiro Hamaji]

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) ?

[Kenichi Ishibashi]

(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!

[Kenichi Ishibashi]

Created attachment 63330 [details]
Patch V1

[Shinichiro Hamaji]

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.

[Stephen White]

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.

[Kenichi Ishibashi]

(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,

[Kenichi Ishibashi]

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.

[WebKit Commit Bot]

Comment on attachment 63330 [details]
Patch V1

Clearing flags on attachment: 63330

Committed r64718: <http://trac.webkit.org/changeset/64718>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.