Closed
Bug 677661
Opened 13 years ago
Closed 13 years ago
Remove nsTPtrArray and add a SafeElementAt(index_type) API to nsTArray when it's instantiated with a pointer type
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
36.12 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
1.64 KB,
patch
|
Details | Diff | Splinter Review |
See the discussion on dev.platform (seems not to be cached by Google Groups yet).
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #551869 -
Flags: review?(jonas)
Assignee | ||
Comment 2•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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...
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Ah, ok, use 'struct' then.
Nice!
Comment 7•13 years ago
|
||
Why not allow 1-arg SafeElementAt for any type that has a default constructor?
Comment 8•13 years ago
|
||
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/89bbee4ec270
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
(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 ;-)
Assignee | ||
Comment 12•13 years ago
|
||
(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•13 years ago
|
||
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
Comment 14•13 years ago
|
||
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.
Description
•