Last Comment Bug 677661 - Remove nsTPtrArray and add a SafeElementAt(index_type) API to nsTArray when it's instantiated with a pointer type
: Remove nsTPtrArray and add a SafeElementAt(index_type) API to nsTArray when i...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-09 12:58 PDT by :Ehsan Akhgari
Modified: 2011-10-18 10:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (35.48 KB, patch)
2011-08-09 12:59 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (36.12 KB, patch)
2011-08-09 13:46 PDT, :Ehsan Akhgari
jonas: review+
Details | Diff | Splinter Review
Something like this (seems to compile but untested) (1.64 KB, patch)
2011-08-10 01:46 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-08-09 12:58:32 PDT
See the discussion on dev.platform (seems not to be cached by Google Groups yet).
Comment 1 :Ehsan Akhgari 2011-08-09 12:59:42 PDT
Created attachment 551869 [details] [diff] [review]
Patch (v1)
Comment 2 :Ehsan Akhgari 2011-08-09 13:46:01 PDT
Created attachment 551879 [details] [diff] [review]
Patch (v2)

This version of the patch actually compiles.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-09 14:36:38 PDT
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 :Ehsan Akhgari 2011-08-09 15:41:15 PDT
(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 2011-08-09 15:46:53 PDT
Ah, ok, use 'struct' then.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-09 16:35:54 PDT
Nice!
Comment 7 neil@parkwaycc.co.uk 2011-08-09 17:02:04 PDT
Why not allow 1-arg SafeElementAt for any type that has a default constructor?
Comment 8 neil@parkwaycc.co.uk 2011-08-10 01:46:13 PDT
Created attachment 552038 [details] [diff] [review]
Something like this (seems to compile but untested)
Comment 10 :Ehsan Akhgari 2011-08-10 08:52:56 PDT
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 neil@parkwaycc.co.uk 2011-08-10 09:21:35 PDT
(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 :Ehsan Akhgari 2011-08-16 07:28:17 PDT
(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] 2011-10-18 10:49:48 PDT
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] 2011-10-18 10:56:35 PDT
Added some more info here: https://developer.mozilla.org/en/XPCOM_array_guide#Bounds-safe_access_to_elements

Note You need to log in before you can comment on or make changes to this bug.