Bug 32828 - WTF should have an arraysize macro
Summary: WTF should have an arraysize macro
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 36252 48191 (view as bug list)
Depends on: 48208
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-21 10:28 PST by Ojan Vafai
Modified: 2010-10-24 16:20 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.01 KB, patch)
2010-10-23 15:01 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (91.91 KB, patch)
2010-10-23 15:53 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (100.08 KB, patch)
2010-10-23 16:07 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (97.95 KB, patch)
2010-10-23 18:18 PDT, Patrick R. Gansterer
ddkilzer: review-
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
JavaScriptCore part (9.32 KB, patch)
2010-10-24 07:37 PDT, Patrick R. Gansterer
ddkilzer: review-
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
JavaScriptCore part (7.52 KB, patch)
2010-10-24 09:46 PDT, Patrick R. Gansterer
ddkilzer: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
JavaScriptCore part (7.47 KB, patch)
2010-10-24 15:30 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2009-12-21 10:28:17 PST
Makes for better readability and lower likelihood of accidental errors.
Comment 1 Darin Adler 2009-12-21 10:34:21 PST
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.
Comment 2 Darin Adler 2010-10-04 11:42:02 PDT
*** Bug 36252 has been marked as a duplicate of this bug. ***
Comment 3 Alexey Proskuryakov 2010-10-23 12:53:12 PDT
*** Bug 48191 has been marked as a duplicate of this bug. ***
Comment 4 Patrick R. Gansterer 2010-10-23 15:01:31 PDT
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.
Comment 5 WebKit Review Bot 2010-10-23 15:04:23 PDT
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.
Comment 6 Patrick R. Gansterer 2010-10-23 15:53:58 PDT
Created attachment 71656 [details]
Patch

Now with "all" changes.
Comment 7 WebKit Review Bot 2010-10-23 15:57:39 PDT
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.
Comment 8 Patrick R. Gansterer 2010-10-23 16:07:26 PDT
Created attachment 71657 [details]
Patch

Argh! But now, with style fix for PluginDatabaseWin.
Comment 9 WebKit Review Bot 2010-10-23 16:11:29 PDT
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.
Comment 10 Early Warning System Bot 2010-10-23 16:14:25 PDT
Attachment 71656 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4671051
Comment 11 Early Warning System Bot 2010-10-23 16:32:39 PDT
Attachment 71657 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4634040
Comment 12 Patrick R. Gansterer 2010-10-23 18:18:17 PDT
Created attachment 71663 [details]
Patch
Comment 13 WebKit Review Bot 2010-10-23 18:23:51 PDT
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.
Comment 14 Early Warning System Bot 2010-10-23 18:47:20 PDT
Attachment 71663 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4616045
Comment 15 Eric Seidel (no email) 2010-10-24 00:12:48 PDT
Attachment 71663 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4709049
Comment 16 Eric Seidel (no email) 2010-10-24 06:30:47 PDT
Attachment 71663 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4734034
Comment 17 David Kilzer (:ddkilzer) 2010-10-24 07:22:14 PDT
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)];
Comment 18 Patrick R. Gansterer 2010-10-24 07:31:44 PDT
(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;
Comment 19 Patrick R. Gansterer 2010-10-24 07:37:56 PDT
Created attachment 71680 [details]
JavaScriptCore part

Now only the JavaScriptCore part and renamed ARRAY_LEGTH to WTF_ARRAY_LENGTH
Comment 20 WebKit Review Bot 2010-10-24 07:39:26 PDT
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.
Comment 21 David Kilzer (:ddkilzer) 2010-10-24 08:16:53 PDT
(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.  :)
Comment 22 Patrick R. Gansterer 2010-10-24 08:26:48 PDT
(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.
Comment 23 David Kilzer (:ddkilzer) 2010-10-24 08:53:35 PDT
(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!
Comment 24 David Kilzer (:ddkilzer) 2010-10-24 09:02:31 PDT
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.
Comment 25 Patrick R. Gansterer 2010-10-24 09:14:24 PDT
(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.
Comment 26 Patrick R. Gansterer 2010-10-24 09:46:54 PDT
Created attachment 71690 [details]
JavaScriptCore part

Removed WinCE _countof changes
Comment 27 Eric Seidel (no email) 2010-10-24 10:03:01 PDT
Attachment 71680 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4620075
Comment 28 David Kilzer (:ddkilzer) 2010-10-24 14:26:58 PDT
Comment on attachment 71690 [details]
JavaScriptCore part

r=me
Comment 29 WebKit Commit Bot 2010-10-24 14:29:21 PDT
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
Comment 30 Patrick R. Gansterer 2010-10-24 15:30:34 PDT
Created attachment 71698 [details]
JavaScriptCore part
Comment 31 WebKit Commit Bot 2010-10-24 16:20:21 PDT
Comment on attachment 71698 [details]
JavaScriptCore part

Clearing flags on attachment: 71698

Committed r70425: <http://trac.webkit.org/changeset/70425>
Comment 32 WebKit Commit Bot 2010-10-24 16:20:30 PDT
All reviewed patches have been landed.  Closing bug.