Closed Bug 963056 Opened 6 years ago Closed 6 years ago

Make MOZ_ARRAY_LENGTH a typesafe compile-time constant, even on MSVC 2010

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: neil, Assigned: poiru)

Details

Attachments

(2 files, 3 obsolete files)

The current definition of MOZ_ARRAY_LENGTH is a compile-time constant but uses the unsafe sizeof construct on platforms that don't support constexpr, while mozilla::ArrayLength is typesafe but not a compile-time constant on those platforms.

The following definition of MOZ_ARRAY_LENGTH is both typesafe and a compile-time constant even without constexpr.

template<int N> struct sized { char dummy[N]; };
template<typename T, int N> sized<N> helper(T(&)[N]);
#define MOZ_ARRAY_LENGTH(array) sizeof(helper(array))
Fantastic trick, Neil!

Birunthan, are you interested in taking this?
Chromium uses something similar:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))
(In reply to Reuben Morais from comment #2)
> Chromium uses something similar:
> 
> template <typename T, size_t N>
> char (&ArraySizeHelper(T (&array)[N]))[N];
> #define arraysize(array) (sizeof(ArraySizeHelper(array)))

Aha, I did wonder how to return a reference to an array of char, turns out I'd put my parentheses in the wrong place. (I should have persevered with cdecl.)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> Birunthan, are you interested in taking this?

Sure.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=24f01cfc7466
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8365214 - Flags: review?(jwalden+bmo)
Comment on attachment 8365214 [details] [diff] [review]
Make MOZ_ARRAY_LENGTH a typesafe compile-time constant on compilers without constexpr support

Why not do this everywhere?  There shouldn't be much advantage in keeping the MOZ_HAVE_CXX11_CONSTEXPR code path around.
Attachment #8365214 - Flags: review?(jwalden+bmo) → review-
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Comment on attachment 8365214 [details] [diff] [review]
> Make MOZ_ARRAY_LENGTH a typesafe compile-time constant on compilers without
> constexpr support
> 
> Why not do this everywhere?  There shouldn't be much advantage in keeping
> the MOZ_HAVE_CXX11_CONSTEXPR code path around.

Fixed, thanks!
Attachment #8365214 - Attachment is obsolete: true
Attachment #8365239 - Flags: review?(jwalden+bmo)
Comment on attachment 8365239 [details] [diff] [review]
Make MOZ_ARRAY_LENGTH a typesafe compile-time constant on compilers without constexpr support

> #endif /* __cplusplus */
> 
> /*
>  * MOZ_ARRAY_LENGTH() is an alternative to mozilla::ArrayLength() for C files
>  * that can't use C++ template functions and for static_assert() calls that
>  * can't call ArrayLength() when it is not a C++11 constexpr function.
>  */
>+#ifdef __cplusplus
Can we eliminate these two preprocessor directives perhaps?
Attachment #8365239 - Flags: review?(jwalden+bmo) → review+
Keywords: checkin-needed
Oh, right -- you can't use templates with arrays of unnamed structs, structs defined within a function, etc.  In such cases -- which can only be determined at the call site -- you have to fall back to sizeof division.
Sure, but gcc 4.7 doesn't seem to care; it must be because it's only 4.4 that it complains.
Ah, yes, gcc didn't pick that bit of C++11 support up until 4.4.

http://wiki.apache.org/stdcxx/C%2B%2B0xCompilerSupport
Then only gcc 4.4 should fall back to sizeof division.
Why not just name the type in question?
This seems to be the only case where this is needed based on a green B2G ICS try push: https://tbpl.mozilla.org/?tree=Try&rev=7b421630c4a7
Attachment #8373390 - Flags: review?(ehsan)
Flags: needinfo?(birunthan)
Comment on attachment 8373390 [details] [diff] [review]
Name unnamed struct in IRC_Composite_C_R0195-incl.cpp

Review of attachment 8373390 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/blink/IRC_Composite_C_R0195-incl.cpp
@@ +416,5 @@
>    {/* IRC_Composite_C_R0195_T000_P090.wav */
>     {-1,1,-1,1,-1,1,-1,2,-2,2,-3,3,-3,4,-5,5,-7,8,-10,14,-25,-385,-133,-356,-344,-390,-312,-438,-400,-852,-107,-995,-245,-186,1358,-771,1473,28,-1710,-3762,-3025,4119,8200,5927,-785,1558,2250,1837,2098,631,-318,150,-77,1215,-23,792,865,600,981,659,366,950,511,766,502,516,589,299,331,571,105,609,462,485,538,594,470,640,393,432,325,604,259,355,336,454,33,263,182,-24,-124,121,-231,-50,-93,49,-230,-101,25,-196,-123,-21,-160,-161,-132,-136,-155,-268,-121,-146,-311,-112,-328,-201,-220,-280,-214,-304,-233,-227,-250,-242,-334,-256,-283,-393,-267,-285,-332,-331,-224,-295,-279,-171,-151,-361,-199,-239,-227,-317,-168,-211,-370,-182,-150,-343,-204,-192,-224,-319,-203,-137,-357,-146,-223,-237,-276,-162,-201,-262,-186,-209,-147,-267,-46,-252,-162,-146,-182,-199,-143,-218,-204,-108,-250,-163,-90,-241,-137,-125,-122,-272,-111,-188,-211,-160,-160,-164,-201,-231,-114,-175,-180,-212,-172,-181,-129,-130,-77,-150,-114,-39,12,-122,-9,-11,-46,-21,-4,7,-73,-20,-50,-21,-80,-102,2,-103,-66,-67,-8,-41,-119,-26,29,-208,3,-108,-34,-228,114,-196,-84,-28,1,-109,-92,77,-169,-27,48,-12,-79,3,-100,-44,-53,13,-103,-90,-14,-91,-23,-64,-21,-116}};
>  
> +namespace mozilla {
> +namespace detail {

Please don't put this in the mozilla::detail namespace, this is code we borrowed from Blink.  I would say don't put it in a namespace at all.
Attachment #8373390 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/a4cafb01c7ff
https://hg.mozilla.org/mozilla-central/rev/9e8948c060ce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.