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)
Core
General
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)
12.88 KB,
patch
|
cpeterson
:
review+
|
Details | Diff | Splinter Review |
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)))
Comment 1•8 years ago
|
||
(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.
Reporter | ||
Comment 2•8 years ago
|
||
Igor said he is interested in working on this PR_ARRAY_SIZE bug.
Assignee: nobody → palmieri.igor
Comment 3•8 years ago
|
||
(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)
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)
Reporter | ||
Comment 6•8 years ago
|
||
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+
Reporter | ||
Comment 7•8 years ago
|
||
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.
Reporter | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
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)
Reporter | ||
Comment 11•8 years ago
|
||
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+
Reporter | ||
Comment 13•8 years ago
|
||
(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)
Reporter | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8786189 -
Flags: review?(martin.thomson) → review+
Comment 17•8 years ago
|
||
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
Reporter | ||
Comment 18•8 years ago
|
||
Thanks again, Igor!
Comment 19•8 years ago
|
||
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
Reporter | ||
Comment 20•8 years ago
|
||
> 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)
Assignee | ||
Comment 21•8 years ago
|
||
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)
Reporter | ||
Comment 22•8 years ago
|
||
LGTM, but let's run one more Try build, just to be sure. :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f648d2141e99
Reporter | ||
Comment 23•8 years ago
|
||
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+
Comment 24•8 years ago
|
||
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
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fcda39e5626
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 26•8 years ago
|
||
Many thanks! I would like to keep working on more bugs, so if anyone have a suggestion, feel free to ask!
Reporter | ||
Comment 27•8 years ago
|
||
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.
Description
•