677661 - Remove nsTPtrArray and add a SafeElementAt(index_type) API to nsTArray when it's instantiated with a pointer type

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

See the discussion on dev.platform (seems not to be cached by Google Groups yet).

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[(no longer active)]

Attachment: Patch (v2)

This version of the patch actually compiles.

Comment 3

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

Comment on attachment 551879 [details] [diff] [review]
Patch (v2)

Review of attachment 551879 [details] [diff] [review]:
-----------------------------------------------------------------

The template magic is crazy! Crazy like a fox!

Though maybe rename nsTPtrArray_helper to nsTArray_SafeElementAtHelper or some such so that it can be specialized for other types.

::: xpcom/glue/nsTArray.h
@@ +128,5 @@
> +  const elem_type& SafeElementAt(index_type i) const;
> +};
> +
> +template <class E, class Derived>
> +struct nsTPtrArray_helper<E*, Derived>

I have to admit that I'm not 100% sure that the template specialization syntax is correct, but if it compiles then it should be :)

I also personally prefer to use 'struct's only for POD types, but that's a personal preference.

Comment 4

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #3)
> The template magic is crazy! Crazy like a fox!

It's true!

> Though maybe rename nsTPtrArray_helper to nsTArray_SafeElementAtHelper or
> some such so that it can be specialized for other types.

Sure, will do.

> ::: xpcom/glue/nsTArray.h
> @@ +128,5 @@
> > +  const elem_type& SafeElementAt(index_type i) const;
> > +};
> > +
> > +template <class E, class Derived>
> > +struct nsTPtrArray_helper<E*, Derived>
> 
> I have to admit that I'm not 100% sure that the template specialization
> syntax is correct, but if it compiles then it should be :)

The syntax is correct, I assure you.  :-)

I'll post it to the try server in any case just to reassure myself!

> I also personally prefer to use 'struct's only for POD types, but that's a
> personal preference.

I can do that, but that would be against the style used in that file...

Comment 5

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

Ah, ok, use 'struct' then.

Comment 6

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

Nice!

Comment 7

[[e-mail]]

Why not allow 1-arg SafeElementAt for any type that has a default constructor?

Comment 8

[[e-mail]]

Attachment: Something like this (seems to compile but untested)

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89bbee4ec270

Comment 10

[(no longer active)]

Comment on attachment 552038 [details] [diff] [review]
Something like this (seems to compile but untested)

Hmm, this doesn't do what you want it to.  It enables a single argument SafeElementAt for any value which can be constructed from 0.  Which for example includes a class like this:

class Buffer {
public: Buffer(int size);
};

(note that the ctor is not expliciit).

Now, defining SafeElementAt as:

elem_type& SafeElementAt(index_type i) {
  return i < Length() ? Elements()[i] : E();
}

might do what you want under some circustances, but 1) I'm not sure what that would mean for built-in types (are they zero-constructed?), and 2) I'm not sure if this is actually the semantics that we want, since the returned value would be a valid type E, and there is no way for the caller to differentiate it from an E() value stored in the array for real.

Comment 11

[[e-mail]]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Hmm, this doesn't do what you want it to.  It enables a single argument
> SafeElementAt for any value which can be constructed from 0.
I actually had it as E() originally, but I wasn't sure what that returns for primitive types, and didn't have time to check.

> elem_type& SafeElementAt(index_type i) {
>   return i < Length() ? Elements()[i] : E();
> }
No, I deliberately didn't want to return a reference.

> I'm not sure if this is actually the semantics that we want, since the
> returned value would be a valid type E, and there is no way for the caller
> to differentiate it from an E() value stored in the array for real.
Then they should use the 2-arg version ;-)

Comment 12

[(no longer active)]

(In reply to comment #11)
> > I'm not sure if this is actually the semantics that we want, since the
> > returned value would be a valid type E, and there is no way for the caller
> > to differentiate it from an E() value stored in the array for real.
> Then they should use the 2-arg version ;-)

If you think this is worth having, please file a new bug.

Comment 13

[Eric Shepherd [:sheppy]]

This is documented (at least mentioned, which is about as good as it gets right now until the array guide is really finished):

https://developer.mozilla.org/en/XPCOM_array_guide

Also mentioned on Firefox 8 for developers.

Comment 14

[Eric Shepherd [:sheppy]]

Added some more info here: https://developer.mozilla.org/en/XPCOM_array_guide#Bounds-safe_access_to_elements