Bug 32828 – WTF should have an arraysize macro

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Ojan Vafai]

Makes for better readability and lower likelihood of accidental errors.

[Darin Adler]

Seems fine. I'd prefer to use a function template rather than a macro. With a  function template I think you can make it a compile time error to accidentally use it on a pointer or on an argument of array type without a size. I'd also suggest not using the name arraysize because that's all lower case and not our normal style.

[Darin Adler]

*** Bug 36252 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 48191 has been marked as a duplicate of this bug. ***

[Patrick R. Gansterer]

Created attachment 71654 [details]
Patch

It's a big patch, but should be easy to review.
Please let me know if I should split it up.

[WebKit Review Bot]

Attachment 71654 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/plugins/win/PluginDatabaseWin.cpp:409:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/StdLibExtras.h:91:  array_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:100:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 71656 [details]
Patch

Now with "all" changes.

[WebKit Review Bot]

Attachment 71656 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/plugins/win/PluginDatabaseWin.cpp:409:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/StdLibExtras.h:91:  array_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:100:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 3 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 71657 [details]
Patch

Argh! But now, with style fix for PluginDatabaseWin.

[WebKit Review Bot]

Attachment 71657 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:91:  array_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/StdLibExtras.h:100:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 2 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 71656 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4671051

[Early Warning System Bot]

Attachment 71657 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4634040

[Patrick R. Gansterer]

Created attachment 71663 [details]
Patch

[WebKit Review Bot]

Attachment 71663 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:94:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 83 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 71663 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4616045

[Eric Seidel (no email)]

Attachment 71663 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4709049

[Eric Seidel (no email)]

Attachment 71663 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4734034

[David Kilzer (:ddkilzer)]

Comment on attachment 71663 [details]
Patch

r- due to various build failures.

I'm a little concerned about ARRAY_LENGTH() being too generic of a name.  What about WTF_ARRAY_LENGTH()?

Can you make the macro an inline template method instead, or does that make declarations like this unhappy?

    unsigned int toklen[ARRAY_LENGTH(tokens)];

[Patrick R. Gansterer]

(In reply to comment #17)
> (From update of attachment 71663 [details])
> r- due to various build failures.
I'll split it up into smaller patches, since the JavaScriptCore part builds already and there is no need to this all in one patch.

> I'm a little concerned about ARRAY_LENGTH() being too generic of a name.  What about WTF_ARRAY_LENGTH()?
I'm not very happy with it too.

> Can you make the macro an inline template method instead, or does that make declarations like this unhappy?
There are some problem, since we need the size at compile time:
E.g. the following won't work:
template <typename T, size_t N>
inline size_t array_size(const T (&)[N])
{
    return N;
}
size_t s = array_size(tokens);

But this should work, if we like it more:
template <typename T, size_t N>
struct array_info<T[N]>
{
    enum { size = N };
};
size_t s = array_info<tokens>::size;

[Patrick R. Gansterer]

Created attachment 71680 [details]
JavaScriptCore part

Now only the JavaScriptCore part and renamed ARRAY_LEGTH to WTF_ARRAY_LENGTH

[WebKit Review Bot]

Attachment 71680 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/StdLibExtras.h:94:  bitwise_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> > Can you make the macro an inline template method instead, or does that make declarations like this unhappy?
> There are some problem, since we need the size at compile time:
> E.g. the following won't work:
> template <typename T, size_t N>
> inline size_t array_size(const T (&)[N])
> {
>     return N;
> }
> size_t s = array_size(tokens);
> 
> But this should work, if we like it more:
> template <typename T, size_t N>
> struct array_info<T[N]>
> {
>     enum { size = N };
> };
> size_t s = array_info<tokens>::size;

Does the struct consume any space when compiled?  Or is it acting like a class here and declaring a scoped enum within itself?  I tend to prefer type-safe code over macros as long as there aren't any bad side-effects to using it.  :)

[Patrick R. Gansterer]

(In reply to comment #21)
> Does the struct consume any space when compiled? Or is it acting like a class here and declaring a scoped enum within itself?  
It only declares the value for the enum, so it shoudn't consume any space, but I'm not 100% sure.

> I tend to prefer type-safe code over macros as long as there aren't any bad side-effects to using it.  :)
Because the macro uses a template function it's typesafe too. It throws a compiler error when used with wrong type (e.g. char*).
IMHO ARRAY_LEGTH(tokens) is easier to read compared to array_info<tokens>::size.

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> (In reply to comment #21)
> > I tend to prefer type-safe code over macros as long as there aren't any bad side-effects to using it.  :)
> Because the macro uses a template function it's typesafe too. It throws a compiler error when used with wrong type (e.g. char*).
> IMHO ARRAY_LEGTH(tokens) is easier to read compared to array_info<tokens>::size.

I agree.  Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 71680 [details]
JavaScriptCore part

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.

[Patrick R. Gansterer]

(In reply to comment #24)
> (From update of attachment 71680 [details])
> > 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?
It's not really used at the moment, so it won't be a problem, but it's cleaner to remove it with the WebCore changes.

> > 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.
Created bug 48208.

[Patrick R. Gansterer]

Created attachment 71690 [details]
JavaScriptCore part

Removed WinCE _countof changes

[Eric Seidel (no email)]

Attachment 71680 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4620075

[David Kilzer (:ddkilzer)]

Comment on attachment 71690 [details]
JavaScriptCore part

r=me

[WebKit Commit Bot]

Comment on attachment 71690 [details]
JavaScriptCore part

Rejecting patch 71690 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 71690]" exit_code: 2
Last 500 characters of output:
preter.cpp
patching file JavaScriptCore/runtime/DatePrototype.cpp
patching file JavaScriptCore/runtime/JSGlobalObject.cpp
patching file JavaScriptCore/runtime/JSONObject.cpp
patching file JavaScriptCore/runtime/UString.cpp
patching file JavaScriptCore/wtf/DateMath.cpp
patching file JavaScriptCore/wtf/StdLibExtras.h
patch: **** malformed patch at line 15:  


Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'David Kilzer', u'--force']" exit_code: 2

Full output: http://queues.webkit.org/results/4727051

[Patrick R. Gansterer]

Created attachment 71698 [details]
JavaScriptCore part

[WebKit Commit Bot]

Comment on attachment 71698 [details]
JavaScriptCore part

Clearing flags on attachment: 71698

Committed r70425: <http://trac.webkit.org/changeset/70425>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.