Closed Bug 1296180 Opened 8 years ago Closed 8 years ago

Replace more uses of PR_ARRAY_SIZE with mozilla::ArrayLengh

Categories

(Core :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: cpeterson, Assigned: palmieri.igor, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

We want to replace NSPR's PR_ARRAY_SIZE macro with MFBT's mozilla::ArrayLength() function.

According to the following code search, there are only 13 remaining use of PR_ARRAY_SIZE in Mozilla's C++ code. C code in the nsprpub/ and security/nss/ directories may use PR_MAX and should not be changed.

http://searchfox.org/mozilla-central/search?q=symbol:M_937945009752fd9a2ab66f72931edc1d2d355141&redirect=false

dom/crypto/CryptoKey.cpp
364 PR_ARRAY_SIZE(keyTemplate));
800 PR_ARRAY_SIZE(keyTemplate));
843 PR_ARRAY_SIZE(keyTemplate));

dom/crypto/WebCryptoCommon.h
109 PR_ARRAY_SIZE(id_ecDH) };
117 PR_ARRAY_SIZE(dhKeyAgreement) };

media/mtransport/transportlayerdtls.cpp
742 for (size_t i = 0; i < PR_ARRAY_SIZE(EnabledCiphers); ++i) {
752 for (size_t i = 0; i < PR_ARRAY_SIZE(DisabledCiphers); ++i) {

security/certverifier/ExtendedValidation.cpp
1276    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
1297    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
1314    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
1403    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {

security/pkix/lib/pkixnss.cpp
219 PR_ARRAY_SIZE(ErrorTableText)

media/webrtc/signaling/test/signaling_unittests.cpp
74  (container<type>((array), (array) + PR_ARRAY_SIZE(array)))
(In reply to Chris Peterson [:cpeterson] from comment #0)
> security/certverifier/ExtendedValidation.cpp
> 1276    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
> 1297    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {
> 1403    for (size_t iEV = 0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {

For these cases, using ranged based for loops instead of ArrayLength() is IMO preferable:
```
for (const nsMyTrustedEVInfo& entry : myTrustedEVInfos) {
  [...]
}
```
None of these cases care about order or the index, so less visual clutter is nice.
Igor said he is interested in working on this PR_ARRAY_SIZE bug.
Assignee: nobody → palmieri.igor
(In reply to Chris Peterson [:cpeterson] from comment #0)
> security/pkix/lib/pkixnss.cpp
> 219 PR_ARRAY_SIZE(ErrorTableText)

Sorry I didn't notice this earlier - anything under security/pkix was at least in the past meant to be separately buildable, and hence the code could not reference anything outside the directory. If this is still the case, this line should not be replaced.

keeler - Is this the separately buildable thing still a goal for mozilla::pkix?
Status: NEW → ASSIGNED
Flags: needinfo?(dkeeler)
Yes, I think for the moment this is still a goal, so it would be nice to not introduce a dependency on mfbt.
Flags: needinfo?(dkeeler)
Attached patch bug-1296180.patch (obsolete) — Splinter Review
Hi,

I am attaching a first patch here. Some important remarks:

- I didn't #include'd ArrayUtils.h in any file that was changed, since it is not needed. Please inform me if I should do that.

- I substituted all but one for's of the list by range based loops (including the ones in transportlayerdtls.cpp).

- within the for of ExtendedValidation.cpp:1403, iEV is actually used, so I only substituted PR_ARRAY_SIZE, not the whole for.

- there are two cases in signaling_unittests.cpp with zero sized arrays (lines 3562 and 3626). Example:

> const char *feedbackTypes[] = { };
>  TestRtcpFbOffer(ARRAY_TO_SET(std::string, feedbackTypes),

Apparently the template deduction used in ArrayLength() doesn't work for N=0, so I kept PR_ARRAY_SIZE. As far as I known, zero sized arrays shouldn't be supported. Can someone please me give advice what would be the better thing to do in this case?

Many thanks!
Attachment #8785518 - Flags: review?(cpeterson)
Comment on attachment 8785518 [details] [diff] [review]
bug-1296180.patch

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

Thanks! Looks good to me, but here are a couple suggestions:

(In reply to Igor from comment #5)
> - I didn't #include'd ArrayUtils.h in any file that was changed, since it is
> not needed. Please inform me if I should do that.

You should #include "mozilla/ArrayUtils.h" even though the code compiles now. Gecko C++ code is usually compiled in "unified mode", where all the .cpp files in each directory are #included into one big .cpp file. This improves build times by reducing redundant header file parsing, but inadvertently allows a header file #included in file a.cpp to be used in b.cpp. However, a few build configurations still compile in "non-unified mode" and can break when b.cpp because it's missing a header file from a.cpp.


> - there are two cases in signaling_unittests.cpp with zero sized arrays
> (lines 3562 and 3626). Example:
> 
> > const char *feedbackTypes[] = { };
> >  TestRtcpFbOffer(ARRAY_TO_SET(std::string, feedbackTypes),
> 
> Apparently the template deduction used in ArrayLength() doesn't work for
> N=0, so I kept PR_ARRAY_SIZE. As far as I known, zero sized arrays shouldn't
> be supported. Can someone please me give advice what would be the better
> thing to do in this case?

Try MOZ_ARRAY_LENGTH, which is #defined in ArrayUtils.h. MOZ_ARRAY_LENGTH is pretty much the same macro as PR_ARRAY_SIZE, but it removes one more dependency on the NSPR library.

::: dom/crypto/CryptoKey.cpp
@@ +360,5 @@
>      { CKA_VALUE,            value->data,          value->len },
>    };
>  
>    mPrivateKey = PrivateKeyFromPrivateKeyTemplate(keyTemplate,
> +                                                 mozilla::ArrayLength(keyTemplate));

>80

::: media/mtransport/transportlayerdtls.cpp
@@ +738,5 @@
>        return false;
>      }
>    }
>  
> +  for (auto const& cipher : EnabledCiphers) {

Maybe use `const auto&` instead of `auto const&`? You use `const nsMyTrustedEVInfo&` below, which is the more common ordering.
Attachment #8785518 - Flags: review?(cpeterson) → feedback+
Comment on attachment 8785518 [details] [diff] [review]
bug-1296180.patch

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

I started a Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dab3a5a0812

::: dom/crypto/CryptoKey.cpp
@@ +360,5 @@
>      { CKA_VALUE,            value->data,          value->len },
>    };
>  
>    mPrivateKey = PrivateKeyFromPrivateKeyTemplate(keyTemplate,
> +                                                 mozilla::ArrayLength(keyTemplate));

I forgot to elaborate: some of these lines are now longer than 80 characters wide. Maybe try using just `ArrayLength` without the `mozilla::` prefix. If that doesn't work, then don't worry about the line length. :)
Thanks for the feedback, Chris.

I've managed to address the issues you mentioned, but I am still having trouble with the zero sized arrays in signaling_unittests.cpp. 

MOZ_ARRAY_LENGTH is defined to use a helper that is templatized on N too:

> #ifdef __cplusplus
> #  define MOZ_ARRAY_LENGTH(array)   sizeof(mozilla::detail::ArrayLengthHelper(array))
> #else
> #  define MOZ_ARRAY_LENGTH(array)   (sizeof(array)/sizeof((array)[0]))
> #endif

and

> template <typename T, size_t N>
> char (&ArrayLengthHelper(T (&array)[N]))[N];

Which still gives me error when N=0. I tried to overload this helper, and ArrayLength() too, but with no success. Does anybody have a good idea here?

That's the only issue remaining to finish the patch.

Thanks again.
(In reply to Igor from comment #8)
> I've managed to address the issues you mentioned, but I am still having
> trouble with the zero sized arrays in signaling_unittests.cpp. 
> 
> MOZ_ARRAY_LENGTH is defined to use a helper that is templatized on N too:
> 
> > #ifdef __cplusplus
> > #  define MOZ_ARRAY_LENGTH(array)   sizeof(mozilla::detail::ArrayLengthHelper(array))
> > #else
> > #  define MOZ_ARRAY_LENGTH(array)   (sizeof(array)/sizeof((array)[0]))
> > #endif

I didn't realize MOZ_ARRAY_LENGTH still used ArrayLengthHelper() in C++. In that case, let's just leave this PR_ARRAY_SIZE.

If this was truly the last use of PR_ARRAY_SIZE or last dependency on the NSPR library, I would suggest we copy PR_ARRAY_SIZE into the test's ARRAY_TO_STL macro. However, there are still other uses of PR_ARRAY_SIZE in Gecko code, so removing this use is not worth duplicating the ARRAY_SIZE code.
Attached patch bug-1296180.patch (obsolete) — Splinter Review
I generated a new patch with the corrections. Please let me know if there is anything else that could be done.

Thanks!
Attachment #8785518 - Attachment is obsolete: true
Attachment #8786189 - Flags: review?(cpeterson)
Comment on attachment 8786189 [details] [diff] [review]
bug-1296180.patch

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

Thanks, Igor. Looks good to me.

David, can you please review Igor's patch to remove the last few uses of NSPR's PR_ARRAY_SIZE macro (which mostly happen to be in crypto code) or recommend another reviewer?
Attachment #8786189 - Flags: review?(dkeeler)
Attachment #8786189 - Flags: review?(cpeterson)
Attachment #8786189 - Flags: feedback+
Comment on attachment 8786189 [details] [diff] [review]
bug-1296180.patch

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

Great - r=me for everything except transportlayerdtls.cpp (I think :mt or :ekr would be good reviewers for that file).
The one thing I would ask is that you sort the #includes where you added mozilla/ArrayUtils.h.

::: media/mtransport/transportlayerdtls.cpp
@@ -734,1 @@
>      rv = SSL_SetSRTPCiphers(ssl_fd, &srtp_ciphers_[0], srtp_ciphers_.size());

I think :mt or :ekr would be good reviewers for this file.
Attachment #8786189 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #12)
> The one thing I would ask is that you sort the #includes where you added
> mozilla/ArrayUtils.h.

David, do you mean sort all the #include lines in the file, not just newly-inserted mozilla/ArrayUtils.h?

> #include "mozilla/dom/ToJSValue.h"
> #include "mozilla/ArrayUtils.h"

Also, should ArrayUtils.h precede dom/ToJSValue.h (because 'A' comes before 'd') or should dom/ToJSValue.h precede ArrayUtils.h (because directory names come before file names)?

Igor, here is the section of Mozilla Coding Style Guide about header file #include ordering:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

Includes are split into three blocks and are sorted alphabetically in each block:
    The main header: Foo.h in Foo.cpp
    Standard library includes: #include <map>
    Mozilla includes: #include "mozilla/dom/Element.h"
Flags: needinfo?(dkeeler)
Comment on attachment 8786189 [details] [diff] [review]
bug-1296180.patch

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

Martin, can you please review Igor's patch to transportlayerdtls.cpp? David Keeler has already r+'d Igor's changes to the dom/crypto files in this patch.

Igor is removing the last few uses of NSPR's PR_ARRAY_SIZE macro. He also changed some C-style for loops that used PR_ARRAY_SIZE to ranged for loops, as Cykesiopka suggested in comment 1.
Attachment #8786189 - Flags: review?(martin.thomson)
(In reply to Chris Peterson [:cpeterson] from comment #13)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #12)
> > The one thing I would ask is that you sort the #includes where you added
> > mozilla/ArrayUtils.h.
> 
> David, do you mean sort all the #include lines in the file, not just
> newly-inserted mozilla/ArrayUtils.h?

Yep, all of them - thanks.

> > #include "mozilla/dom/ToJSValue.h"
> > #include "mozilla/ArrayUtils.h"
> 
> Also, should ArrayUtils.h precede dom/ToJSValue.h (because 'A' comes before
> 'd') or should dom/ToJSValue.h precede ArrayUtils.h (because directory names
> come before file names)?

I use vim, so I usually make a visual selection and then run `:sort`. For me, the order here would be ArrayUtils and then dom/ToJSValue. Unfortunately the style guide isn't extremely specific as to what "sorted alphabetically" means (is it case sensitive? insensitive? I've seen both, although usually no precedence is given to directories vs. files.) If your editor has a built-in sorting function, whatever it comes up with is probably fine.
Flags: needinfo?(dkeeler)
Many thanks for the tips and for pointing out the coding style. I was wondering about where to put the #includes since the beginning. 

I generated a new patch with all the headers fixed. I used emacs sort-lines, which as by my tests behaves as vim :sort (and they are case sensitive). I didn't marked the previous patch as obsolete because there are some flags set on it.

If there is any other suggestion or feedback, feel free to ask me.
Attachment #8787030 - Flags: review+
Attachment #8786189 - Flags: review?(martin.thomson) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb6e4e4060f
Replace more uses of PR_ARRAY_SIZE with mozilla::ArrayLengh. r=mt
Thanks again, Igor!
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/18f49d6c092d for Windows (or perhaps only Win64, that's all that has burned so far) warning-as-error bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=35609864&repo=mozilla-inbound

 23:14:07     INFO -  c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/WebCryptoCommon.h(110): error C2220: warning treated as error - no 'object' file generated
 23:14:07     INFO -  Warning: C4838 in c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla\dom\WebCryptoCommon.h: conversion from 'size_t' to 'unsigned int' requires a narrowing conversion
 23:14:07     INFO -  c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/WebCryptoCommon.h(110): warning C4838: conversion from 'size_t' to 'unsigned int' requires a narrowing conversion
 23:14:07     INFO -  c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/WebCryptoCommon.h(110): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
 23:14:07     INFO -  Warning: C4838 in c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla\dom\WebCryptoCommon.h: conversion from 'size_t' to 'unsigned int' requires a narrowing conversion
 23:14:07     INFO -  c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/WebCryptoCommon.h(118): warning C4838: conversion from 'size_t' to 'unsigned int' requires a narrowing conversion
 23:14:07     INFO -  c:\builds\moz2_slave\m-in-w64-000000000000000000000\build\src\obj-firefox\dist\include\mozilla/dom/WebCryptoCommon.h(118): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
 23:14:07     INFO -  c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/config/rules.mk:950: recipe for target 'Unified_cpp_dom_media_webrtc0.obj' failed
 23:14:07 INFO - mozmake.EXE[5]: *** [Unified_cpp_dom_media_webrtc0.obj] Error 2
> dom\WebCryptoCommon.h: conversion from 'size_t' to 'unsigned int' requires a narrowing conversion

Igor, it looks like your patch broke the Windows build. The problem is that ArrayLength() returns type size_t but the SECItem struct's `len` member variable is of type unsigned int [1], which may be smaller than size_t. We don't want to change SECItem, so you can just suppress the warning with a cast like static_cast<unsigned int>(ArrayLength()).

[1] http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/security/nss/lib/util/seccomon.h#54
Flags: needinfo?(palmieri.igor)
Makes sense. But why it didn't show in the Try tests?

I updated the patch with the static_cast<> in WebCryptoCommon.h, and tried to keep a reasonable indentation, but I am no sure it is good enough.

Please let me know if there is anything else that I can do to help.
Attachment #8786189 - Attachment is obsolete: true
Attachment #8787030 - Attachment is obsolete: true
Flags: needinfo?(palmieri.igor)
Attachment #8789792 - Flags: review?(cpeterson)
LGTM, but let's run one more Try build, just to be sure. :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f648d2141e99
Comment on attachment 8789792 [details] [diff] [review]
bug-1296180.patch

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

Try looks good. I'll land this patch, carrying over David's and Martin's r+.
Attachment #8789792 - Flags: review?(cpeterson) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fcda39e5626
Replace more uses of PR_ARRAY_SIZE with mozilla::ArrayLength. r=keeler,mt
https://hg.mozilla.org/mozilla-central/rev/1fcda39e5626
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Many thanks!

I would like to keep working on more bugs, so if anyone have a suggestion, feel free to ask!
Igor, there are some new good bugs on "Bugs Ahoy" here:

http://www.joshmatthews.net/bugsahoy/?cpp=1&unowned=1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: