Closed Bug 785918 Opened 7 years ago Closed 7 years ago

Replace the usages of PR_ARRAY_SIZE with mozilla::ArrayLength

Categories

(Core :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed

People

(Reporter: ehsan, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(4 files, 1 obsolete file)

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.
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/
(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?
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.
Hmm, OK.  Josh, do you know of anyone who is willing to do the investigative work here?
Hi!

On nsprpub/pr/include/prtypes.h we can find a define for this macro. Can we remove it? Or is necessary maintain it?
(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.
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?
If the change causes a build error, we're probably stuck with the macro.
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?
(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! :-)
Attachment #663778 - Flags: review?(ehsan)
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!
Attachment #663778 - Flags: review?(ehsan) → feedback+
Assignee: nobody → guilherme.sft
Status: NEW → ASSIGNED
That's it! Where can I found more information about what source I can change and what they do?
Attachment #664325 - Flags: review?(ehsan)
(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.
Attachment #664325 - Flags: review?(ehsan) → review+
(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!
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.
Attachment #699020 - Flags: review?(ehsan)
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. :)
Attachment #699021 - Flags: review?(jwalden+bmo)
(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 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.  :-)
Attachment #699020 - Flags: review?(ehsan) → review+
Comment on attachment 699021 [details] [diff] [review]
part-2-MOZ_HAVE_CXX11_CONSTEXPR.patch

You should not use MOZ_ALWAYS_INLINE here.
I landed part 1 on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f052fe6615
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][leave open]
Remove inline directive from MOZ_HAVE_CXX11_CONSTEXPR macro.
Attachment #699021 - Attachment is obsolete: true
Attachment #699021 - Flags: review?(jwalden+bmo)
Attachment #699839 - Flags: review?(jwalden+bmo)
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.
Attachment #699839 - Flags: review?(jwalden+bmo) → review+
(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?
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.  :-\
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb032d63b415
Assignee: guilherme.sft → cpeterson
Whiteboard: [mentor=ehsan][lang=c++][leave open] → [mentor=ehsan][lang=c++]
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/eb032d63b415
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1277775
Depends on: 1296180
You need to log in before you can comment on or make changes to this bug.