Add nsTPtrArray, which has a SafeElementAt method




11 years ago
11 years ago


(Reporter: bz, Assigned: sicking)



Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)

I'd like to move some table code to nsTArray, but it makes extensive use of nsVoidArray::SafeElementAt.  Now SafeElementAt doesn't make sense for nsTArray, but I'd like to propose:

template <class T>
nsTPtrArray : public nsTArray<T*>
  nsTPtrArray() {}
  explicit nsTPtrArray(size_type capacity) :
    nsTArray<T*>(capacity) {}

  T* SafeElementAt(index_type i) {
    if (i >= Length()) {
      return nsnull;

    return ElementAt(i);

I talked with darin about SafeElementAt before, and the idea came up to add a function like

nsTArray::SafeElementAt(index_type i, Item& item);

I.e. you'd be able to pass in the 'default' value returned. However I this bug is a good idea too, since it could simply forward to the SafeElementAt on nsTArray.
The default-value SafeElementAt would be fine with me, sure.  Let's do that.


11 years ago
Blocks: 356335
Assignee: nobody → bugmail
Created attachment 242018 [details] [diff] [review]
Patch to fix

I'll call your nsTPtrArray and raise one nsTConstPtrArray.
Attachment #242018 - Flags: superreview?(bzbarsky)
Attachment #242018 - Flags: review?(bzbarsky)
Comment on attachment 242018 [details] [diff] [review]
Patch to fix

sr=me, and r=me on the content/xslt changes, but an xpcom peer or someone should review the array changes...
Attachment #242018 - Flags: superreview?(bzbarsky)
Attachment #242018 - Flags: superreview+
Attachment #242018 - Flags: review?(darin)
Attachment #242018 - Flags: review?(bzbarsky)

Comment 5

11 years ago
Comment on attachment 242018 [details] [diff] [review]
Patch to fix

At first glance, it isn't obvious why nsTConstPtrArray exists.  Might be good to add some comments above it to point that out.

Are there any things to unit test for these classes?
Attachment #242018 - Flags: review?(darin) → review+
Yeah, I could add some tests for the SafeElementAt functions, want a new patch for that or should I just check it in?
Created attachment 242098 [details] [diff] [review]
Patch v2

The original patch didn't work at all in VC6 and caused a lot of warnings in GCC3.2. The new patch contains the following changes:

1. Explicitly typedef the inherited types since the compiler doesn't otherwise
   know that |elem_type| is a typename (rather than, say, a member) in the
   inherited class.
   This is required since we're inheriting a templetized class

2. Make SafeElementAt(index_type) return by value rather than by reference.
   Otherwise you could do something like
   myArray.SafeElementAt(big_nr) = x;
   which would try to assign into nsnull (and probably fail to compile)

3. Kill nsTConstPtrArray. I realized that nsTPtrArray<const int> works just as

4. Add unit tests.
Attachment #242018 - Attachment is obsolete: true
Attachment #242098 - Flags: superreview?(bzbarsky)
Attachment #242098 - Flags: review?(darin)
Comment on attachment 242098 [details] [diff] [review]
Patch v2

Attachment #242098 - Flags: superreview?(bzbarsky) → superreview+


11 years ago
Attachment #242098 - Flags: review?(darin) → review+
Checked in. I had to add some |typename| declarations to the typedefs to get it compiling everywhere.
Last Resolved: 11 years ago
Resolution: --- → FIXED
Isn't it possible to create a specialization of nsTArray that adds the extra method to nsTArray<T*> directly?
Can you do that such that it applies to all pointers?

And wouldn't we need to copy the entire implementation of nsTArray to that specialization? Though we could create an nsTArray_base2 that both the generic and specialized version inherit from.
Created attachment 252407 [details]

I'm not saying that this works in all compilers, but this example builds fine with gcc and shows that you can have extra methods for all template instances of pointer type. It also shows that you can even have those instances extend another instance of the same template, although I don't necessarily recommend this...
The problem with that implementation is that you have to override all functions since otherwise you'll have the wrong datatype in the signatures, like you did with get(), no? I.e. what you really want is to inherit Foo<T*>.

However I think that can be solved by having a separate base class. I.e. do something like:

template<T> nsTArray_base2<T> : public nsTArray_base {
  ...current implementation goes here...

template<T> nsTArray<T> : public nsTArray_base2<T> {}

template<T> nsTArray<T*> : public nsTArray_base2<T*> {
  SafeElementAt(index_type index) {
However I'm not really convinced that this is buying us anything. But if you make sure it works everywhere I wouldn't oppose checking it in :)
What it buys us is that we don't have to deal with yet another array type.

The thinking behind using a non-parameterized base class is that it might reduce code size. All arrays of pointers should be able to share the code for their methods. I think we'd achieve that if the array-of-pointer class is just inline wrappers that cast to/from void* and call the single base class. I'm not sure that other approaches achieve that.
While you'd be able to reduce the compiled code, you would need to copy the entire source of nsTArray, no? Or at least create a wrapper function for each function that does the appropriate casting.

As far as far as reducing binary size there is actually a lot we can do with the code as it is. I started some work on this a while ago, but I wanted to wait until we have more call sites so that we can measure the impact before going too crazy with it since some of it is a neatness vs. codesize tradeoff.
Right, I was thinking "wrapper functions".

It would be easiest if you could make nsTArray<T*> inherit from nsTArray<void*>. But you'd need to stop the nsTArray<T*> template from applying to T=void. this is possible, but only by using tricks that I would consider dirty (OTOH the Boost people would say they're bread-and-butter :-) ).
Maybe this is something we can look in to once we start working harder on reducing the compile size of nsTArray.
You need to log in before you can comment on or make changes to this bug.