355754 - Add nsTPtrArray, which has a SafeElementAt method

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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*>
{
public:
  nsTPtrArray() {}
  explicit nsTPtrArray(size_type capacity) :
    nsTArray<T*>(capacity) {}

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

    return ElementAt(i);
  }
}

Thoughts?

Comment 1

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

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The default-value SafeElementAt would be fine with me, sure.  Let's do that.

Comment 3

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

Attachment: Patch to fix

I'll call your nsTPtrArray and raise one nsTConstPtrArray.

Comment 4

[Boris Zbarsky [: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...

Comment 5

[Darin Fisher]

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?

Comment 6

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

Yeah, I could add some tests for the SafeElementAt functions, want a new patch for that or should I just check it in?

Comment 7

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

Attachment: 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
   well.

4. Add unit tests.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 242098 [details] [diff] [review]
Patch v2

sr=bzbarsky

Comment 9

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

Checked in. I had to add some |typename| declarations to the typedefs to get it compiling everywhere.

Comment 10

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

Isn't it possible to create a specialization of nsTArray that adds the extra method to nsTArray<T*> directly?

Comment 11

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

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.

Comment 12

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

Attachment: example

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...

Comment 13

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

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) {
    ...
  }
}

Comment 14

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

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 :)

Comment 15

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

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.

Comment 16

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

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.

Comment 17

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

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 :-) ).

Comment 18

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

Maybe this is something we can look in to once we start working harder on reducing the compile size of nsTArray.