Closed Bug 828300 Opened 7 years ago Closed 6 years ago

Replace the usages of NS_ARRAY_LENGTH with mozilla::ArrayLength and MOZ_ARRAY_LENGTH

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Ms2ger, Assigned: poiru)

References

Details

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

Attachments

(3 files, 6 obsolete files)

+++ 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.
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>.
Sakshi, you expressed interest in this bug. Please feel free to follow up here.
Hello,

I have built Firefox.
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?
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.
Okay, I will try.
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 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 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)
Including the inputs from Ms2ger and ehsan.
Attachment #707537 - Attachment is obsolete: true
Attachment #708035 - Flags: review?(ehsan)
Attachment #708035 - Flags: review?(Ms2ger)
Assignee: nobody → gaurav.pruthi88
Status: NEW → ASSIGNED
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+
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.
(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.)
Attachment #708035 - Flags: review?(Ms2ger)
Can you confirm that you're still working on this bug?
Flags: needinfo?(gaurav.pruthi88)
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)
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
I would like to continue the bug. I will upload the patch with all the changes. Thanks !
Assignee: nobody → amod.narvekar
Attached patch Patchv1 (obsolete) — Splinter Review
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 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+
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
(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
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.
(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!
Attachment #757307 - Flags: feedback?(kbrosnan)
I will resume the work from July 20 since I am away from my hometown.
I am facing problems to upload the patch to try server. Can you please do the needful ? Thanks !
Flags: needinfo?(ehsan)
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)
ok. I will do within few days. Sorry for late reply
Flags: needinfo?(amod.narvekar)
Assigning myself as Amod does not seem to be working on this any longer.
Assignee: amod.narvekar → birunthan
Attachment #708035 - Attachment is obsolete: true
Attachment #757307 - Attachment is obsolete: true
Attachment #8361920 - Flags: review?(ehsan)
Attachment #8361920 - Flags: review?(ehsan) → review+
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+
Depends on: 962088
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 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 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+
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+
Attachment #8361921 - Attachment is obsolete: true
Attachment #8368031 - Flags: review+
Attachment #8368029 - Flags: review+
Keywords: checkin-needed
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][checkin attachment 8368025 to comm-central before m-c patches]
https://hg.mozilla.org/mozilla-central/rev/f33daf2f8fb4
https://hg.mozilla.org/mozilla-central/rev/26028496b508
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.