Closed Bug 356299 Opened 18 years ago Closed 16 years ago

Implement nsAutoTArray<T,N>

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: sicking)

Details

Attachments

(1 file, 7 obsolete files)

It'd be nice to have a typesafe replacement for nsAutoVoidArray.  roc suggested nsAutoTArray<T,N> (with space for N elements of type T).
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?
I'm not working on it.  ;)  But I'd like to use it once we have it!
Assignee: nobody → bugmail
Attached patch Patch to fix (obsolete) — Splinter Review
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.
Attachment #242000 - Flags: superreview?(dbaron)
Attachment #242000 - Flags: review?(darin)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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
Attachment #242000 - Attachment is obsolete: true
Attachment #242002 - Flags: superreview?(dbaron)
Attachment #242002 - Flags: review?(darin)
Attachment #242000 - Flags: superreview?(dbaron)
Attachment #242000 - Flags: review?(darin)
Hmm.. should the name be nsTAutoArray or nsAutoTArray?
I prefer nsAutoTArray myself, but nsTAutoArray is consistent with nsCString.
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
Attachment #242002 - Flags: superreview?(dbaron)
Attachment #242002 - Flags: review?(darin)
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #242002 - Attachment is obsolete: true
Attachment #242027 - Flags: superreview?(dbaron)
Attachment #242027 - Flags: review?(darin)
Attachment #242027 - Flags: review?(darin) → review+
Attached patch Patch 2.1 (obsolete) — Splinter Review
This syncs to tip and adds nsAutoTPtrArray too now that we have nsTPtrArray.
Attachment #242027 - Attachment is obsolete: true
Attachment #242027 - Flags: superreview?(dbaron)
Gonna get this tested on a few compilers before I ask for review
Attached patch Patch v2.2 (obsolete) — Splinter Review
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
Attachment #242580 - Attachment is obsolete: true
Attachment #242672 - Flags: superreview?(darin)
Attachment #242672 - Flags: review?(darin)
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.
super_class sounds good then. I'll make the same change to nsTPtrArray
Hmm.. On second thought base_type sounds better to me.
Attached patch Patch v2.3 (obsolete) — Splinter Review
Changed parent_type to base_type.
Attachment #242672 - Attachment is obsolete: true
Attachment #242792 - Flags: superreview?(darin)
Attachment #242792 - Flags: review?(darin)
Attachment #242672 - Flags: superreview?(darin)
Attachment #242672 - Flags: review?(darin)
Comment on attachment 242792 [details] [diff] [review]
Patch v2.3

Just found a bug with this patch. New one comming up
Attachment #242792 - Flags: superreview?(darin)
Attachment #242792 - Flags: review?(darin)
Attached patch Patch v2.4 (obsolete) — Splinter Review
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.
Attachment #242792 - Attachment is obsolete: true
Attachment #242803 - Flags: superreview?(darin)
Attachment #242803 - Flags: review?(darin)
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.
Attachment #242803 - Flags: superreview?(darin.moz)
Attachment #242803 - Flags: review?(darin.moz)
Attachment #242803 - Flags: review-
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.
Attached patch Patch v2.6Splinter Review
Attachment #242803 - Attachment is obsolete: true
Attachment #244185 - Flags: superreview?
Attachment #244185 - Flags: review?
Attachment #244185 - Flags: superreview?(darin.moz)
Attachment #244185 - Flags: superreview?
Attachment #244185 - Flags: review?(darin.moz)
Attachment #244185 - Flags: review?
Attachment #244185 - Flags: superreview?(darin.moz)
Attachment #244185 - Flags: superreview+
Attachment #244185 - Flags: review?(darin.moz)
Attachment #244185 - Flags: review+
This is in after a bumpy landing.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
We have code that use it so it _must_ work :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.