258018 - replace nsUInt32Array with nsTArray<PRUint32>

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

See bug 250811

Comment 1

[Nick Kreeger]

I'm taking a look at working around this code in Correo, and I figured I would fix this while I'm at it. 

Instead of using |nsValueArray|, my proposal is to use |nsTArray| instead.

Taking...

Comment 2

[David :Bienvenu]

I don't know if you can completely replace nsUint32Array, but you might base it on nsTArray - we rely on things like sorting and searching a sorted nsUint32Array, especially in nsMsgKeyArray.

Comment 3

[[e-mail]]

(In reply to comment #1)
>Instead of using |nsValueArray|, my proposal is to use |nsTArray| instead.
Yeah, nsValueArray is going away...

Comment 4

[[e-mail]]

Attachment: nsAddrBookSession patch

Comment 5

[[e-mail]]

Attachment: nsMsgMailSession patch

Comment 6

[David :Bienvenu]

Comment on attachment 295088 [details] [diff] [review]
nsAddrBookSession patch

thx for the patch, Neil.

Comment 7

[[e-mail]]

Attachment: nsImapUndoTxn patch

Comment 8

[[e-mail]]

Attachment: nsLocalUndoTxn patch

Comment 9

[[e-mail]]

Attachment: MapiMessage patch

Comment 10

[[e-mail]]

Comment on attachment 295151 [details] [diff] [review]
MapiMessage patch

>+  DWORD aNum = m_attachNums[idx];
Might as well make it an nsTArray<DWORD> then - see bug 410479 ;-)
Given that bug, I don't think I can fairly review this.

Comment 11

[[e-mail]]

Attachment: GetSelectedIndices patch

I had to make a bunch of pointers const because Elements() is. I should probably reformat all the affected lines while I'm there...

I removed the sorting because tree selections are already sorted.

Comment 12

[[e-mail]]

Comment on attachment 295678 [details] [diff] [review]
GetSelectedIndices patch

>Index: nsMsgDBView.cpp
>===================================================================
>@@ -1098,13 +1098,13 @@
>-nsresult nsMsgDBView::GetSelectedIndices(nsUInt32Array *selection)
>+nsresult nsMsgDBView::GetSelectedIndices(nsMsgViewIndexArray& selection)
> {
>   if (mTreeSelection)
>   {
>     PRInt32 count;
>     mTreeSelection->GetCount(&count);
>-    selection->SetSize(count);
>+    selection.SetLength(count);
>     count = 0;
>     PRInt32 selectionCount;
>     nsresult rv = mTreeSelection->GetRangeCount(&selectionCount);
>@@ -1118,7 +1118,7 @@
>       if (startRange >= 0 && startRange < viewSize)
>       {
>         for (PRInt32 rangeIndex = startRange; rangeIndex <= endRange && rangeIndex < viewSize; rangeIndex++)
>-          selection->SetAt(count++, rangeIndex);
>+          selection[count++] = rangeIndex;

doing a SetLength() call followed by assigning all the elements is efficient, but SetLength() won't zero the data like nsUInt32Array did. so if there's an early return (http://mxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgDBView.cpp#1116) and your callers don't check the return value of GetSelectedIndices() (they don't) then this is bad, no?

SetCapacity() is safer, but i'll go along with SetLength() provided you expand out that NS_ENSURE_SUCCESS to Clear() the array on failure.

>@@ -2128,17 +2128,16 @@
>-  nsUInt32Array selection;
>-  GetSelectedIndices(&selection);
>-  *length = selection.GetSize();
>-  PRUint32 numIndicies = *length;
>-  if (!numIndicies) return NS_OK;
>+  nsMsgViewIndexArray selection;
>+  GetSelectedIndices(selection);
>+  PRUint32 numIndices = selection.Length();
>+  if (!numIndices) return NS_OK;
>+  *length = numIndices;

want to make the same *length/spelling fixes...

>@@ -2151,9 +2150,9 @@
>-  nsUInt32Array selection;
>-  GetSelectedIndices(&selection);
>-  *length = selection.GetSize();
>+  nsMsgViewIndexArray selection;
>+  GetSelectedIndices(selection);
>+  *length = selection.Length();
>   PRUint32 numIndicies = *length;
>   if (!numIndicies) return NS_OK;

here?

r=me with those!

Comment 13

[[e-mail]]

as a side note, nsAutoTArray inherits from nsTArray, so if you have code like

nsAutoTArray<foo, 2> bar;
baz(bar);

then baz can just have a signature
void baz(nsTArray<foo>& aBar);

but since you instantiate your array in a bunch of different places the typedef makes sense.

Comment 14

[[e-mail]]

(In reply to comment #12)
> >@@ -2128,17 +2128,16 @@
> >-  nsUInt32Array selection;
> >-  GetSelectedIndices(&selection);
> >-  *length = selection.GetSize();
> >-  PRUint32 numIndicies = *length;
> >-  if (!numIndicies) return NS_OK;
> >+  nsMsgViewIndexArray selection;
> >+  GetSelectedIndices(selection);
> >+  PRUint32 numIndices = selection.Length();
> >+  if (!numIndices) return NS_OK;
> >+  *length = numIndices;
> 
> want to make the same *length/spelling fixes...

er wait, on second look, is that right? *length won't get set to 0 before returning.

Comment 15

[[e-mail]]

(In reply to comment #14)
>*length won't get set to 0 before returning.
Don't worry, the out parameters are cleared early on (not enough -u, I know...)

Comment 16

[[e-mail]]

Attachment: GetSelectedIndices patch

* Updated for bitrot
* Removed the consts (I only added them because I misread nsTArray.h)
* Retained the removal of the sorting (although I thought I had to remove them so that I could add the consts, they're not actually needed anyway)

Comment 17

[David :Bienvenu]

Comment on attachment 300330 [details] [diff] [review]
GetSelectedIndices patch

I wish I remember why were sorting the view indices, but it looks like you're right that we don't need to.

Comment 18

[[e-mail]]

Attachment: Most of the rest

Most of this is just boilerplate substitution, but somehow I couldn't let
PartitionSelectionByFolder off the hook without a rewrite. At one point I
even considered storing the indices in an nsTArray<nsTArray<PRUint32> > .

Comment 19

[Jay Moran (jaym/lazvon)]

(In reply to comment #18)

I applied the patch this morning and recompiled and turned the Remember Message preference again. No crash after two hours worth of testing. I haven't been run that long with normal mail usage in the past 12 days. So far I like it. Will post results after a full day in a few hours; but if others can apply the diff and test I think you'd appreciate the results! :)

Comment 20

[Jay Moran (jaym/lazvon)]

(In reply to comment #19)

ARGH! WRONG BUG!!! Comment 19 is not valid for this bug! :)

Comment 21

[David :Bienvenu]

Comment on attachment 300713 [details] [diff] [review]
Most of the rest

thx, Neil.

Can you just remove these comment lines?

         //			m_idArray.RemoveAll();
-        //			m_flags.RemoveAll();
+        //			m_flags.Clear();

Comment 22

[[e-mail]]

Attachment: Fix includes

The only remaining consumer of nsUInt32Array is nsMsgKeyArray, which is being dealt with in a separate bug. Even then we can't simply remove the header file because three files are relying on it to include prmem.h; this patch fixes that.

For nsMessenger.cpp the allocations are all wrong so I fixed them.
For nsMsgDBView.cpp we need to be able to free the Allocate(d)RawSortKey.
For nsMsgUtils.cpp the ns_MsgSA* functions should be replaced by nsCString...

Comment 23

[[e-mail]]

Attachment: Fix more includes

Including some more allocator mismatches too.

Comment 24

[David :Bienvenu]

Comment on attachment 303271 [details] [diff] [review]
Fix more includes

getting rid of PR_CALLOC and +1 alloc size looks OK.

Comment 25

[[e-mail]]

This bug is now fixed.

Bug 413413 will actually remove nsUInt32ARray.* as nsMsgKeyArray still uses it.