Closed
Bug 828300
Opened 11 years ago
Closed 10 years ago
Replace the usages of NS_ARRAY_LENGTH with mozilla::ArrayLength and MOZ_ARRAY_LENGTH
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: Ms2ger, Assigned: poiru)
References
Details
(Whiteboard: [mentor=ehsan][lang=c++][qa-])
Attachments
(3 files, 6 obsolete files)
17.26 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
60.23 KB,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
1018 bytes,
patch
|
poiru
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #785918 +++ See <https://mxr.mozilla.org/mozilla-central/search?string=NS_ARRAY_LENGTH>. Where possible, these users need to be changed to use the mozilla::ArrayLength inline function; otherwise (in C or for static assertions), they should use MOZ_ARRAY_LENGTH.
Hello there, My friend and I are interested in helping out. Is this bug still active? If so, where/how do we begin? Thanks.
Comment 2•11 years ago
|
||
It is, yes! Have you built Firefox before? If not, the best place to start is to setup your build environment and get an initial build going. See <https://developer.mozilla.org/En/Simple_Firefox_build>.
Comment 3•11 years ago
|
||
Sakshi, you expressed interest in this bug. Please feel free to follow up here.
As mentioned in the previous comment, I have looked up some of the files which use NS_ARRAY_LENGTH. Could you please tell me how I could proceed?
Comment 6•11 years ago
|
||
Modify the files to use mozilla::ArrayLength when possible, or MOZ_ARRAY_LENGTH when it's not. Fix any build errors, then submit a patch.
Comment 8•11 years ago
|
||
Attachment #707537 -
Flags: review?(ehsan)
Reporter | ||
Comment 9•11 years ago
|
||
Comment on attachment 707537 [details] [diff] [review] Attaching the proposed patch for replacement of NS_ARRAY_LENGTH with ArrayLength() and MOZ_ARRAY_LENGTH Review of attachment 707537 [details] [diff] [review]: ----------------------------------------------------------------- Henri, I suppose you need changes to the translation code for the changes to the HTML parser here? ::: dom/system/gonk/AutoMounter.cpp @@ +210,5 @@ > { > VolumeManager::RegisterStateObserver(&mVolumeManagerStateObserver); > Volume::RegisterObserver(&mVolumeEventObserver); > > + for (size_t i = 0; i < MOZ_ARRAY_LENGTH(sAutoVolumeName); i++) { ArrayLength ::: dom/system/gonk/SystemWorkerManager.cpp @@ +177,5 @@ > } > > memcpy(JS_GetArrayBufferViewData(array), mMessage->mData, mMessage->mSize); > jsval argv[] = { OBJECT_TO_JSVAL(array) }; > + return JS_CallFunctionName(aCx, obj, "onRILMessage", MOZ_ARRAY_LENGTH(argv), ArrayLength @@ +324,5 @@ > } > > memcpy(JS_GetUint8ArrayData(array), mMessage->mData, mMessage->mSize); > jsval argv[] = { OBJECT_TO_JSVAL(array) }; > + return JS_CallFunctionName(aCx, obj, "onNetdMessage", MOZ_ARRAY_LENGTH(argv), ArrayLength ::: toolkit/components/mediasniffer/nsMediaSniffer.cpp @@ +87,5 @@ > } > > const uint32_t clampedLength = NS_MIN(aLength, MAX_BYTES_SNIFFED); > > + for (uint32_t i = 0; i < MOZ_ARRAY_LENGTH(sSnifferEntries); ++i) { ArrayLength
Attachment #707537 -
Flags: review?(hsivonen)
Comment 10•11 years ago
|
||
Comment on attachment 707537 [details] [diff] [review] Attaching the proposed patch for replacement of NS_ARRAY_LENGTH with ArrayLength() and MOZ_ARRAY_LENGTH r+ on the changes under parser/html/. I'll change the translator.
Attachment #707537 -
Flags: review?(hsivonen) → review+
Comment 12•11 years ago
|
||
Comment on attachment 707537 [details] [diff] [review] Attaching the proposed patch for replacement of NS_ARRAY_LENGTH with ArrayLength() and MOZ_ARRAY_LENGTH Review of attachment 707537 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your patch, Gaurav! Please address the comments below, in addition to what Ms2ger already said. ::: extensions/universalchardet/src/base/CharDistribution.cpp @@ +36,5 @@ > > EUCTWDistributionAnalysis::EUCTWDistributionAnalysis() > { > mCharToFreqOrder = EUCTWCharToFreqOrder; > + mTableSize = MOZ_ARRAY_LENGTH(EUCTWCharToFreqOrder); You should be able to use ArrayLength for all of these. ::: gfx/thebes/gfxPangoFonts.cpp @@ +293,5 @@ > > // Prefer HarfBuzz if the font also has support for OpenType shaping of > // this script. > const FcChar8 otCapTemplate[] = "otlayout:XXXX"; > + FcChar8 otCap[ArrayLength(otCapTemplate)]; Hmm, I think you want MOZ_ARRAY_LENGTH here. ::: layout/style/nsCSSParser.cpp @@ +8746,5 @@ > eCSSProperty_list_style_position, > eCSSProperty_list_style_image > }; > > + nsCSSValue values[ArrayLength(listStyleIDs)]; MOZ_ARRAY_LENGTH. ::: layout/style/nsRuleNode.cpp @@ +7890,5 @@ > + > + nsCSSProperty properties[ArrayLength(backgroundValues) + > + ArrayLength(borderValues) + > + ArrayLength(paddingValues) + > + ArrayLength(textShadowValues)]; MOZ_ARRAY_LENGTH for all of these. ::: xpcom/tests/TestTArray.cpp @@ +437,5 @@ > // useful in non-debug builds. > #ifdef DEBUG > static bool test_autoarray() { > uint32_t data[] = {4,6,8,2,4,1,5,7,3}; > + nsAutoTArray<uint32_t, ArrayLength(data)> array; MOZ_ARRAY_LENGTH. @@ +442,5 @@ > > void* hdr = array.DebugGetHeader(); > if (hdr == nsTArray<uint32_t>().DebugGetHeader()) > return false; > + if (hdr == nsAutoTArray<uint32_t, ArrayLength(data)>().DebugGetHeader()) Ditto. @@ +484,5 @@ > > array.Compact(); > array.AppendElements(data, ArrayLength(data)); > uint32_t data3[] = {5, 7, 11}; > + nsAutoTArray<uint32_t, ArrayLength(data3)> array3; Here too.
Attachment #707537 -
Flags: review?(ehsan)
Comment 13•11 years ago
|
||
Including the inputs from Ms2ger and ehsan.
Attachment #707537 -
Attachment is obsolete: true
Attachment #708035 -
Flags: review?(ehsan)
Attachment #708035 -
Flags: review?(Ms2ger)
Updated•11 years ago
|
Assignee: nobody → gaurav.pruthi88
Status: NEW → ASSIGNED
Comment 14•11 years ago
|
||
Comment on attachment 708035 [details] [diff] [review] Attaching the updated patch for replacement of NS_ARRAY_LENGTH with ArrayLength() and MOZ_ARRAY_LENGTH Review of attachment 708035 [details] [diff] [review]: ----------------------------------------------------------------- Cool, I think this is good. Have you pushed this to the try server yet?
Attachment #708035 -
Flags: review?(ehsan) → review+
Comment 15•11 years ago
|
||
I think I need Level 1 commit access, but don't know if I need to raise a bug for the same , don't know. If I will, who will vouch. BTW I didn't got any compilation error while building.
Reporter | ||
Comment 16•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a98c6a0f2f2b
Comment 17•11 years ago
|
||
(In reply to comment #15) > I think I need Level 1 commit access, but don't know if I need to raise a bug > for the same , don't know. If I will, who will vouch. BTW I didn't got any > compilation error while building. Please feel free to file a bug and CC me on it. I will be happy to vouch for you. See <http://www.mozilla.org/hacking/committer/> for details on how to do this. Also, there are a bunch of compilation errors with the patch on non-Linux platforms, can you please look into them? (See the link in comment 16.)
Reporter | ||
Updated•11 years ago
|
Attachment #708035 -
Flags: review?(Ms2ger)
Comment 18•11 years ago
|
||
Can you confirm that you're still working on this bug?
Flags: needinfo?(gaurav.pruthi88)
Comment 19•11 years ago
|
||
I'm very sorry for getting late on this. I'm a bit busy with my office work till June. Would be better if you can assign it to somebody else if it needs to get fixed till then.
Flags: needinfo?(gaurav.pruthi88)
Comment 20•11 years ago
|
||
Going to unassign this bug then. When you get free if this bug is open then please comment if you want to work on it again. Thank you for taking a look at this.
Assignee: gaurav.pruthi88 → nobody
Comment 21•11 years ago
|
||
I would like to continue the bug. I will upload the patch with all the changes. Thanks !
Updated•11 years ago
|
Assignee: nobody → amod.narvekar
Comment 22•11 years ago
|
||
Once I get r+ then i would upload it on the try server since I have L1 access. It will be my first time to upload there.
Attachment #757307 -
Flags: review?(ehsan)
Attachment #757307 -
Flags: feedback?(kbrosnan)
Comment 23•11 years ago
|
||
Comment on attachment 757307 [details] [diff] [review] Patchv1 Review of attachment 757307 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Please let me know if you have problems pushing to try.
Attachment #757307 -
Flags: review?(ehsan) → review+
Comment 24•11 years ago
|
||
Yes, I am facing bit problems as follows: >pushing to ssh://hg.mozilla.org/try/ >remote: Permission denied (publickey). >abort: no suitable response from remote hg! Contents of the files: 1. mozilla-central/.hg/hgrc = http://pastebin.mozilla.org/2485217 2. mozilla-central/.hgrc = http://pastebin.mozilla.org/2485227 3. ~/.ssh/config = http://pastebin.mozilla.org/2485228
Comment 25•11 years ago
|
||
(In reply to Amod [:greatwarrior] from comment #24) > 3. ~/.ssh/config = http://pastebin.mozilla.org/2485228 Amod, two likely problems: 1. You might want to double-check that the Try server has been updated with your commit level permissions and public key. 2. I think your ~/.ssh/config file needs to point to your private key. My ssh config has an entry that looks like this: Host *.mozilla.org IdentityFile ~/.ssh/cpeterson@mozilla.com/id_rsa User cpeterson@mozilla.com
Comment 26•11 years ago
|
||
Try running: ssh hg.mozilla.org If that tells you "No interactive shells allowed here!" then that's good news, it means that your ssh keys are properly set up. Otherwise you need to file an IT bug to sort it out. Also, running ssh -vvv hg.mozilla.org will print out debugging information which might help you figure out where the problem is.
Comment 27•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=13b3c2aaa1df Are the results as expected ?
Comment 28•11 years ago
|
||
(In reply to comment #27) > https://tbpl.mozilla.org/?tree=Try&rev=13b3c2aaa1df > > Are the results as expected ? Well, you pushed to the try server successfully, but as you can see all of the builds have failed. Can you please fix the compilation errors and try again? Thanks!
Updated•11 years ago
|
Attachment #757307 -
Flags: feedback?(kbrosnan)
Comment 29•11 years ago
|
||
I will resume the work from July 20 since I am away from my hometown.
Comment 30•11 years ago
|
||
I am facing problems to upload the patch to try server. Can you please do the needful ? Thanks !
Updated•11 years ago
|
Flags: needinfo?(ehsan)
Comment 31•11 years ago
|
||
Amod, can you please upload a new version of your patch? The one that I pushed to try in comment 28 didn't successfully build. Thanks!
Flags: needinfo?(ehsan) → needinfo?(amod.narvekar)
Comment 32•11 years ago
|
||
ok. I will do within few days. Sorry for late reply
Flags: needinfo?(amod.narvekar)
Assignee | ||
Comment 33•10 years ago
|
||
Assigning myself as Amod does not seem to be working on this any longer.
Assignee: amod.narvekar → birunthan
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #708035 -
Attachment is obsolete: true
Attachment #757307 -
Attachment is obsolete: true
Attachment #8361920 -
Flags: review?(ehsan)
Assignee | ||
Comment 35•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=aa77a64a4a80
Attachment #8361921 -
Flags: review?(ehsan)
Updated•10 years ago
|
Attachment #8361920 -
Flags: review?(ehsan) → review+
Comment 36•10 years ago
|
||
Comment on attachment 8361921 [details] [diff] [review] Remove NS_ARRAY_LENGTH Review of attachment 8361921 [details] [diff] [review]: ----------------------------------------------------------------- r=me on doing this some day, but please note that comm-central still uses NS_ARRAY_LENGTH in a bunch of places so landing this patch will break them. Please either fix that too or file a follow-up to do that. Thanks!
Attachment #8361921 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 37•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=294770ea5cc8
Attachment #8363032 -
Flags: review?(Pidgeot18)
Comment 38•10 years ago
|
||
Comment on attachment 8363032 [details] [diff] [review] Replace NS_ARRAY_LENGTH with mozilla::ArrayLength/MOZ_ARRAY_LENGTH in comm-central I'd ask Neil too so see the suite and mailnews stuff outside MIME.
Attachment #8363032 -
Flags: review?(neil)
Comment 39•10 years ago
|
||
Comment on attachment 8363032 [details] [diff] [review] Replace NS_ARRAY_LENGTH with mozilla::ArrayLength/MOZ_ARRAY_LENGTH in comm-central mozilla::ArrayLength isn't constant enough everywhere yet; MOZ_ARRAY_LENGTH is (there's a fallback for compilers without constexpr support which bug 963056 could improve). r=me but I'd prefer it if you used MOZ_ARRAY_LENGTH everywhere.
Attachment #8363032 -
Flags: review?(neil) → review+
Comment 40•10 years ago
|
||
Comment on attachment 8363032 [details] [diff] [review] Replace NS_ARRAY_LENGTH with mozilla::ArrayLength/MOZ_ARRAY_LENGTH in comm-central Review of attachment 8363032 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/shell/nsMailGNOMEIntegration.cpp @@ +135,5 @@ > nsMailGNOMEIntegration::IsDefaultClient(bool aStartupCheck, uint16_t aApps, bool * aIsDefaultClient) > { > *aIsDefaultClient = true; > > + for (unsigned int i = 0; i < ArrayLength(sAppTypes); i++) { This needs to be mozilla::ArrayLength @@ +153,5 @@ > NS_IMETHODIMP > nsMailGNOMEIntegration::SetDefaultClient(bool aForAllUsers, uint16_t aApps) > { > nsresult rv = NS_OK; > + for (unsigned int i = 0; i < ArrayLength(sAppTypes); i++) { Ditto. ::: mailnews/base/search/src/nsMsgFilter.cpp @@ +908,5 @@ > { nsMsgFilterAction::MarkUnread, "Mark unread"}, > { nsMsgFilterAction::Custom, "Custom"}, > }; > > +static const unsigned int sNumActions = mozilla::ArrayLength(ruleActionsTable); MOZ_ARRAY_LENGTH here? ::: mailnews/base/search/src/nsMsgFilterList.cpp @@ +373,5 @@ > {nsIMsgFilterList::attribCustomId, "customId"}, > }; > > static const unsigned int sNumFilterFileAttribTable = > + mozilla::ArrayLength(FilterFileAttribTable); And here. ::: mailnews/base/search/src/nsMsgSearchTerm.cpp @@ +90,5 @@ > {nsMsgSearchAttrib::HasAttachmentStatus, "has attachment status"}, > }; > > static const unsigned int sNumSearchAttribEntryTable = > + mozilla::ArrayLength(SearchAttribEntryTable); ... and here... @@ +235,5 @@ > {nsMsgSearchOp::DoesntMatch, "doesn't match"} > }; > > static const unsigned int sNumSearchOperatorEntryTable = > + mozilla::ArrayLength(SearchOperatorEntryTable); ... and here. ::: mailnews/base/test/TestMsgStripRE.cpp @@ +9,5 @@ > #include "nsIPrefService.h" > #include "nsIPrefBranch.h" > #include "nsISupportsPrimitives.h" > #include "nsStringGlue.h" > +#include "mozilla/ArrayUtils.h" Please move this include between TestHarness.h and nsCOMPtr.h. I'd like to keep the header include lists clean in the few cases where they actually exist. ::: suite/profile/migration/src/nsThunderbirdProfileMigrator.cpp @@ +15,5 @@ > #include "nsNetCID.h" > #include "nsNetUtil.h" > #include "nsThunderbirdProfileMigrator.h" > #include "prprf.h" > +#include "mozilla/ArrayUtils.h" I expect the correct place for this include is right before nsCRT.h
Attachment #8363032 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Comment 41•10 years ago
|
||
Changed to MOZ_ARRAY_LENGTH everywhere as per comment #39. Addresses comment #40 as well.
Attachment #8363032 -
Attachment is obsolete: true
Attachment #8368025 -
Flags: review+
Assignee | ||
Comment 42•10 years ago
|
||
Rebased patch.
Attachment #8361920 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8361921 -
Attachment is obsolete: true
Attachment #8368031 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8368029 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][checkin attachment 8368025 to comm-central before m-c patches]
Comment 44•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/507ea58cbc08 https://hg.mozilla.org/integration/mozilla-inbound/rev/f33daf2f8fb4 https://hg.mozilla.org/integration/mozilla-inbound/rev/26028496b508
Keywords: checkin-needed
Whiteboard: [mentor=ehsan][lang=c++][checkin attachment 8368025 to comm-central before m-c patches] → [mentor=ehsan][lang=c++]
Comment 45•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f33daf2f8fb4 https://hg.mozilla.org/mozilla-central/rev/26028496b508
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•10 years ago
|
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•