785918 - Replace the usages of PR_ARRAY_SIZE with mozilla::ArrayLength

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

http://mxr.mozilla.org/mozilla-central/ident?i=PR_ARRAY_SIZE shows that some places in the toolkit and security modules use the PR_ARRAY_SIZE macro.  They need to be changed to use the mozilla::ArrayLength inline function.

Comment 1

[Jeff Walden [:Waldo]]

Until we compile as C++11, and all compilers support the relevant aspect of C++11 (constexpr, and the ability to instantiate a type that depends on a local type), we can't make this switch everywhere, note.  See the "Limitations" section of this post:

http://whereswalden.com/2011/10/20/computing-the-length-or-end-of-a-compile-time-constant-length-array-jsns_array_endlength-are-out-mozillaarraylengthend-are-in/

Comment 2

[(no longer active)]

(In reply to comment #1)
> Until we compile as C++11, and all compilers support the relevant aspect of
> C++11 (constexpr, and the ability to instantiate a type that depends on a local
> type), we can't make this switch everywhere, note.  See the "Limitations"
> section of this post:
> 
> http://whereswalden.com/2011/10/20/computing-the-length-or-end-of-a-compile-time-constant-length-array-jsns_array_endlength-are-out-mozillaarraylengthend-are-in/

Understood.  Is that why we're using PR_ARRAY_SIZE in those cases, or are you just mentioning this in general?

Comment 3

[Jeff Walden [:Waldo]]

It's the reason we use NS_ARRAY_LENGTH most (perhaps even all) of the places where we use it.  I think I tried to fix up PR_ARRAY_SIZE users at the time as well, but my memory's hazy.

Comment 4

[(no longer active)]

Hmm, OK.  Josh, do you know of anyone who is willing to do the investigative work here?

Comment 5

[José Guilherme Vanz]

Hi!

On nsprpub/pr/include/prtypes.h we can find a define for this macro. Can we remove it? Or is necessary maintain it?

Comment 6

[(no longer active)]

(In reply to guilherme.sft from comment #5)
> Hi!
> 
> On nsprpub/pr/include/prtypes.h we can find a define for this macro. Can we
> remove it? Or is necessary maintain it?

Unfortunately we can't really change NSPR, so that macro definition needs to stay there.

Comment 7

[José Guilherme Vanz]

On toolkit/identity/IdentityCryptoService.cpp we can find a static assert uses PR_ARRAY_SIZE macro. But I can't replace it because will cause compile error.  I maintain the macro? Or is there other way?

Comment 8

[Josh Matthews [:jdm]]

If the change causes a build error, we're probably stuck with the macro.

Comment 9

[José Guilherme Vanz]

Attachment: Some PR_ARRAY_SIZE macro replacement

Comment 10

[Josh Matthews [:jdm]]

Great, José! All patches need review, and in this case I think the mentor, ehsan, would be a good first step, so could you follow the steps at https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews and flag :ehsan to review your patch?

Comment 11

[José Guilherme Vanz]

(In reply to Josh Matthews [:jdm] from comment #10)
> Great, José! All patches need review, and in this case I think the mentor,
> ehsan, would be a good first step, so could you follow the steps at
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch#Getting_reviews and flag :ehsan to review your patch?

Ok! :-)

Comment 12

[(no longer active)]

Comment on attachment 663778 [details] [diff] [review]
Some PR_ARRAY_SIZE macro replacement

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

Thanks a lot for your patch, José.  It looks very good!  Please attach a new version of the patch which addresses my comments below and it will be ready for checking in.  Thanks!

::: nsprpub/pr/tests/sprintf.c
@@ +86,5 @@
>      };
>      int f, s, n, p;
>      char fmt[20];
>  
> +    for (f = 0; f < mozilla::ArrayLength(formats); f++) {

Please revert the changes to this file, as it's part of NSPR and our copy of NSPR is read-only.

::: security/nss/lib/certhigh/certvfypkix.c
@@ +286,5 @@
>      PKIX_CHECK(
>          PKIX_List_Create(&ekuOidsList, plContext),
>          PKIX_LISTCREATEFAILED);
>  
> +    for (;i < mozilla::ArrayLength(certUsageEkuStringMap);i++) {

Please revert this change as well, since this code is part of NSS and our copy is read-only again!

Comment 13

[José Guilherme Vanz]

Attachment: New patch with request reverts in the review

That's it! Where can I found more information about what source I can change and what they do?

Comment 14

[(no longer active)]

(In reply to José Guilherme Vanz from comment #13)
> Created attachment 664325 [details] [diff] [review]
> New patch with request reverts in the review
> 
> That's it! Where can I found more information about what source I can change
> and what they do?

Unfortunately I don't think we have a high level document which describes this in detail.  The biggest chunks of borrowed code are NSPR (in nsprpub/) and NSS (in security/nss/).  You can also check out about:license for the licensing information of the code we have borrowed.

Comment 15

[(no longer active)]

Thanks a lot for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9951a2e6df

Comment 16

[:Benjamin Peterson]

Clang is evidently not happy: https://tbpl.mozilla.org/php/getParsedLog.php?id=15515442&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/4af7c8a2159f

Comment 17

[José Guilherme Vanz]

(In reply to Benjamin Peterson [:benjamin] from comment #16)
> Clang is evidently not happy:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=15515442&tree=Mozilla-
> Inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4af7c8a2159f

Hum... I'll fix it  ;)
Thanks!

Comment 18

[Chris Peterson [:cpeterson]]

Attachment: part-1-MOZ_ARRAY_LENGTH.patch

Part 1: Replace PR_ARRAY_SIZE() with mozilla::ArrayLength() and MOZ_ARRAY_LENGTH(). The MOZ_ARRAY_LENGTH() macro is necessary to support MOZ_STATIC_ASSERTs that are evaluated at compile-time.

Comment 19

[Chris Peterson [:cpeterson]]

Attachment: part-2-MOZ_HAVE_CXX11_CONSTEXPR.patch

Part 2: Add MOZ_HAVE_CXX11_CONSTEXPR check and constexpr version of ArrayLength().

For compilers that support C++11's constexpr functions, we can make the MOZ_ARRAY_LENGTH() macro call mozilla::ArrayLength(), which is more type-safe than the sizeof(array)/sizeof(array[0]) hack.

I split this constexpr change into its own patch because MOZ_ARRAY_LENGTH() patch 1 can work without constexpr and I assumed MOZ_CONSTEXPR might be controversial or draw bikeshedding comments. :)

Comment 20

[Masatoshi Kimura [:emk]]

Please also replace NS_ARRAY_LENGTH.
https://mxr.mozilla.org/mozilla-central/search?string=NS_ARRAY_LENGTH

Comment 21

[(no longer active)]

(In reply to Masatoshi Kimura [:emk] from comment #20)
> Please also replace NS_ARRAY_LENGTH.
> https://mxr.mozilla.org/mozilla-central/search?string=NS_ARRAY_LENGTH

I won't object if you wanna do this in a separate bug.

Comment 22

[(no longer active)]

Comment on attachment 699020 [details] [diff] [review]
part-1-MOZ_ARRAY_LENGTH.patch

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

Please drop the inline annotations.  Also, I'd not wait on the second part before landing this.  :-)

Comment 23

[(no longer active)]

Comment on attachment 699021 [details] [diff] [review]
part-2-MOZ_HAVE_CXX11_CONSTEXPR.patch

You should not use MOZ_ALWAYS_INLINE here.

Comment 24

[Chris Peterson [:cpeterson]]

I landed part 1 on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f052fe6615

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/32f052fe6615

Comment 26

[Chris Peterson [:cpeterson]]

Attachment: part-2-MOZ_HAVE_CXX11_CONSTEXPR-v2.patch

Remove inline directive from MOZ_HAVE_CXX11_CONSTEXPR macro.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 699839 [details] [diff] [review]
part-2-MOZ_HAVE_CXX11_CONSTEXPR-v2.patch

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

::: mfbt/Attributes.h
@@ +131,5 @@
>  
>  /*
> + * The MOZ_CONSTEXPR specifier declares that a C++11 compiler can evaluate a
> + * function at compile time. A constexpr function cannot examine any values
> + * except its arguments and can have no side effects except its return value.

There's more complexity than this to constexpr, because you can mark constructors and static fields as constexpr.  But maybe this is good enough, I guess.

I'm a little worried about adding a macro for this, because it lets people start writing programs that depend on MOZ_CONSTEXPR not being empty.  If after more experience with this we find people are writing programs that do this, we may want to roll back on MOZ_CONSTEXPR.  But I guess we can try it for now.

::: mfbt/Util.h
@@ +276,5 @@
>   */
> +#ifdef MOZ_HAVE_CXX11_CONSTEXPR
> +#  define MOZ_ARRAY_LENGTH(array)   mozilla::ArrayLength(array)
> +#else
> +#  define MOZ_ARRAY_LENGTH(array)   (sizeof(array)/sizeof((array)[0]))

This'll "work" on pointer types as well as array types, which doesn't seem very good.  How about you add this to TypeTraits.h:

#include <stddef.h>

template<typename T>
struct IsArray
{
    static const bool value = false;
};

template<typename T, size_t N>
struct IsArray<T[N]>
{
    static const bool value = true;
};

And add some tests like this to mfbt/tests/TestTypeTraits.cpp:

template<typename T>
static void
test(const char* str)
{
  printf("%s: %d\n", str, int(IsArray<T>::value));
}

struct Q { };

int main()
{
  test<void>("void");
  test<void*>("void*");
  test<int*>("int*");
  test<int>("int");
  test<Q>("Q");
  test<Q[5]>("Q[5]");
  test<char[]>("char[]");
}

And then change the sizeof version here to this (untested):

(sizeof(mozilla::ArrayLength(array)), sizeof(array) / sizeof((array)[0]))

For pedantry I should perhaps note that this macro won't work if array is a class with overloaded operator[].  We could work around this using sizeof(0[array]) (yes, 0[ptr] is actually valid syntax), but it's probably not worth the readability hit.  Hopefully nobody will try using this macro on a C array of Vectors, or something.

Comment 28

[Chris Peterson [:cpeterson]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27)
> I'm a little worried about adding a macro for this, because it lets people
> start writing programs that depend on MOZ_CONSTEXPR not being empty.  If
> after more experience with this we find people are writing programs that do
> this, we may want to roll back on MOZ_CONSTEXPR.  But I guess we can try it
> for now.

I see your point. Where MOZ_CONSTEXR is more than an optimization and is required for a feature to work correctly (such as the constexpr version of mozilla::ArrayLength()), there may not be an easy non-MOZ_CONSTEXPR fallback. However, MSVC does not implement constexpr, so I don't think anyone could land a feature without a non-constexpr option. 


> And then change the sizeof version here to this (untested):
> 
> (sizeof(mozilla::ArrayLength(array)), sizeof(array) / sizeof((array)[0]))

The sizeof version of MOZ_ARRAY_LENGTH() is called from C code (webrtc/signaling) that can't call mozilla::ArrayLength().

Are you proposing three versions of MOZ_ARRAY_LENGTH(): a C++ version with constexpr, a C++ sizeof version without constexpr, and a plain C sizeof version?

Comment 29

[Jeff Walden [:Waldo]]

We're adding more C code that we've written to our tree?  :-(

Go back to what you had, in the meantime, until we can somehow convince them to be C++ using basically the C subset.  :-\

Comment 30

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb032d63b415

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb032d63b415