356299 - Implement nsAutoTArray<T,N>

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It'd be nice to have a typesafe replacement for nsAutoVoidArray.  roc suggested nsAutoTArray<T,N> (with space for N elements of type T).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is very doable without any overhead (code wise and almost speed wise) by using the same trick as I did for nsAutoVoidArray.

I'll take this unless someone else is already working on it?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not working on it.  ;)  But I'd like to use it once we have it!

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like it too!

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This should do it. I'm a bit unhappy about the implementation of nsTArray_base::GetAutoArrayBuffer as it assumes things about alignment of members across classes, and assumes that nsTArray has no non-inherited members. I did add an assertion that makes sure that's the case though.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1.1

Added some comments for the new functions.

Oh, I did notice one problem with nsTArray, unrelated to this patch. At least on windows the following does not compile:

nsTArray<nsINode*> arr;
nsIAttribute* item = ...
arr.AppendElement(item);

The problem is that nsTArrayElementTraits::Construct(E *e, const A &arg) is unable to cast an |nsIAttribute *const| to |nsINode*|. Seems like a compiler bug to me, but something that may be worth working around.

Anyhow, that's a different bug, but the reason for the NS_STATIC_CASTs in nsContentUtils.cpp

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. should the name be nsTAutoArray or nsAutoTArray?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I prefer nsAutoTArray myself, but nsTAutoArray is consistent with nsCString.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242002 [details] [diff] [review]
Patch v1.1

I think I prefer nsAutoTArray too, i'll make that change. Additionally I noticed the nsTArray::SwapElements function which breaks with this patch.

New patch in a sec

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

This should do it. Unfortunately swap became a good bit slower, however it should still be really fast for the case when both arrays are normal nsTArrays. I also added some tests to TestTArray.cpp.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch 2.1

This syncs to tip and adds nsAutoTPtrArray too now that we have nsTPtrArray.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Gonna get this tested on a few compilers before I ask for review

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.2

The following changes have been made to the previously reviewed patch:

1. All references to inherited things in nsAutoTArray (such as mHdr) has to be
   explicitly prefixed with parent_type:: (where parent_type is a typedef to
   nsTArray<E>) to make gcc happy. 

2. Add nsAutoTPtrArray

So the only changes are in the nsAutoT(Ptr)Array classes.

This has been tested on VC6, VC7.2 and gcc4.0.2-8

Comment 13

[Darin Fisher]

Maybe "base_type" or "super_type" instead of "parent_type"?  It seems like it is more common to refer to base/derived or superclass/subclass when speaking of class inheritance.  Java, at least, has a "super" keyword.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

super_class sounds good then. I'll make the same change to nsTPtrArray

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. On second thought base_type sounds better to me.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.3

Changed parent_type to base_type.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242792 [details] [diff] [review]
Patch v2.3

Just found a bug with this patch. New one comming up

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.4

The problem was my change to nsTArray_base::EnsureCapacity to move the
|capacity <= mHdr->mCapacity| test before the |mHdr == &sEmptyHdr| test. The result is that if you add an empty array to another empty array we'll first call EnsureCapacity(0) and then IncrementLength(0).

Before my patch the EnsureCapacity call would allocate a new array (with 0 elements) and when we reach IncrementLength mHdr is pointing to an allocated array. With my patch the EnsureCapacity call is a no-op and so mHdr is still sEmptyHdr when we call IncrementLength(0).

I'm not actually sure if this is a problem or not. We are after all just over writing 0 with 0. However it seems like a bad idea to change a variable that's declared const.

We could either fix this by making sEmptyHdr not be declared as const, or by adding an if-statement to IncrementHeader. Alternatively we could make EnsureCapacity(0) allocate a new buffer again, but I kind of think that appending an empty array should be a no-op.

I went with adding the if-statement, but I'm fine with removing the const too.

Comment 19

[Darin Fisher]

Comment on attachment 242803 [details] [diff] [review]
Patch v2.4

The duplicate of code in nsTArray_base::SwapArrayElements for copying the static array into a heap buffer is not so great.  I'd add a helper function and call that on |this| and |other| instead.  Also, why not fixup the flags at that point, so you can just do the swap of mHdr finally.  I don't understand the code that does the mIsAutoArray twiddling.

At the very least the code taht does the mIsAutoArray twiddling, which appears to be duplicated, should be moved into a helper function.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The first part is a good idea to break out.

The bit twiddling is a bit harder since it depends on the state of both arrays, and after the twiddling is done the arrays are in a broken state until they've had their buffers switched. I'd prefer to add the following comment:

If the two arrays have different is-auto-array values (i.e. one is an autoarray and one is not) then simply switching the buffers is going to make that bit wrong. We therefore adjust these bits before switching the buffers so that once the buffers are switched the is-auto-array bits are right again.
However, we have to watch out so that we don't set the bit on sEmptyHeader. If an array uses the empty header (and the other therefore must be an nsAutoTArray) we make it point to the other arrays autobuffer so that when the buffers are switched the other points to its autobuffer.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.6

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is in after a bumpy landing.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We have code that use it so it _must_ work :)