The default bug view has changed. See this FAQ.

Implement nsAutoTArray<T,N>

VERIFIED FIXED

Status

()

Core
XPCOM
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: bz, Assigned: sicking)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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!
I'd like it too!
Assignee: nobody → bugmail
Created attachment 242000 [details] [diff] [review]
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.
Attachment #242000 - Flags: superreview?(dbaron)
Attachment #242000 - Flags: review?(darin)
Created attachment 242002 [details] [diff] [review]
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
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)
Created attachment 242027 [details] [diff] [review]
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.
Attachment #242002 - Attachment is obsolete: true
Attachment #242027 - Flags: superreview?(dbaron)
Attachment #242027 - Flags: review?(darin)

Updated

11 years ago
Attachment #242027 - Flags: review?(darin) → review+
Created attachment 242580 [details] [diff] [review]
Patch 2.1

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
Created attachment 242672 [details] [diff] [review]
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
Attachment #242580 - Attachment is obsolete: true
Attachment #242672 - Flags: superreview?(darin)
Attachment #242672 - Flags: review?(darin)

Comment 13

11 years ago
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.
Created attachment 242792 [details] [diff] [review]
Patch v2.3

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)
Created attachment 242803 [details] [diff] [review]
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.
Attachment #242792 - Attachment is obsolete: true
Attachment #242803 - Flags: superreview?(darin)
Attachment #242803 - Flags: review?(darin)

Comment 19

11 years ago
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.
Created attachment 244185 [details] [diff] [review]
Patch v2.6
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?

Updated

11 years ago
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.

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 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.