[webkit-reviews] review denied: [Bug 32828] WTF should have an arraysize macro : [Attachment 71680] JavaScriptCore part

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 32828: WTF should have an arraysize macro
https://bugs.webkit.org/show_bug.cgi?id=32828

Attachment 71680: JavaScriptCore part
https://bugs.webkit.org/attachment.cgi?id=71680&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71680&action=review

r- to address the _countof() issue and move style changes and "inline" change
to a separate patch.

> JavaScriptCore/wtf/Platform.h:-570
> -/* _countof is only included in CE6; for CE5 we need to define it ourself */

> -#ifndef _countof
> -#define _countof(x) (sizeof(x) / sizeof((x)[0]))
> -#endif
> -

I still think this is needed for WebCore, right?

> JavaScriptCore/wtf/StdLibExtras.h:94
> +template<typename TO, typename FROM>
> +inline TO bitwise_cast(FROM from)

I would prefer this change be made in a separate patch.  It has potential
performance implications, so making it a separate patch will allow for better
bisecting in the future in case it has some effect.

You can bundle it with the style indentation fixes as well.

> JavaScriptCore/wtf/StdLibExtras.h:111
> +// Returns a count of the number of bits set in 'bits'.
> +inline size_t bitCount(unsigned bits)
> +{
> +    bits = bits - ((bits >> 1) & 0x55555555);
> +    bits = (bits & 0x33333333) + ((bits >> 2) & 0x33333333);
> +    return (((bits + (bits >> 4)) & 0xF0F0F0F) * 0x1010101) >> 24;
> +}

Please put the style changes in a different patch.  You can bundle it with the
style change and "inline" change above.