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

RESOLVED FIXED in mozilla8

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-complete})

Trunk
mozilla8
x86
Mac OS X
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

See the discussion on dev.platform (seems not to be cached by Google Groups yet).
Created attachment 551869 [details] [diff] [review]
Patch (v1)
Attachment #551869 - Flags: review?(jonas)
Created attachment 551879 [details] [diff] [review]
Patch (v2)

This version of the patch actually compiles.
Attachment #551869 - Attachment is obsolete: true
Attachment #551869 - Flags: review?(jonas)
Attachment #551879 - Flags: review?(jonas)
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.
Attachment #551879 - Flags: review?(jonas) → review+
(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...
Keywords: dev-doc-needed
Ah, ok, use 'struct' then.
Nice!

Comment 7

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

Comment 8

6 years ago
Created attachment 552038 [details] [diff] [review]
Something like this (seems to compile but untested)
http://hg.mozilla.org/mozilla-central/rev/89bbee4ec270
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.
(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 ;-)
(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.
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.
Keywords: dev-doc-needed → dev-doc-complete
Added some more info here: https://developer.mozilla.org/en/XPCOM_array_guide#Bounds-safe_access_to_elements
You need to log in before you can comment on or make changes to this bug.