Last Comment Bug 696195 - nsTArray should have SafeElementAt specializations for smart pointers
: nsTArray should have SafeElementAt specializations for smart pointers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 696201
  Show dependency treegraph
 
Reported: 2011-10-20 13:30 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-10-25 18:02 PDT (History)
3 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add single-argument SafeElementAt specializations for nsTArrays of smart pointers. (1.80 KB, patch)
2011-10-20 13:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-10-20 13:30:09 PDT
I want to use SafeElementAt sanely on an nsTArray<nsCOMPtr>.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-10-20 13:49:58 PDT
Created attachment 568503 [details] [diff] [review]
Add single-argument SafeElementAt specializations for nsTArrays of smart pointers.
Comment 2 Justin Lebar (not reading bugmail) 2011-10-20 14:01:37 PDT
Why not have a method which returns an nsCOMPtr&, like the two-arg SafeElementAt methods do?  You could just return an empty nsCOMPtr, right?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-10-21 09:23:49 PDT
I could, I suppose, but are there any use cases for that?
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-21 09:28:36 PDT
Comment on attachment 568503 [details] [diff] [review]
Add single-argument SafeElementAt specializations for nsTArrays of smart pointers.

Instead of hardcoding nsCOMPtr and nsRefPtr you could use a template template parameter, see for example:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#131

The syntax is horrendous, but it works!
Comment 5 Justin Lebar (not reading bugmail) 2011-10-21 09:30:25 PDT
It just seems kinda weird to me that the nsTArray interface uses nsCOMPtr<T>& and nsCOMPtr<T>*, except SafeElementAt, which returns T*.

For a use-case, how about something like:

  nsCOMPtr<Foo>& foo = arr.SafeElementAt(i);
  if (foo && ...) {
    // null out the element in the array, but don't remove it, because that'd be expensive.
    return foo.forget();
  }
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-10-21 09:32:59 PDT
> The syntax is horrendous, but it works!

It also seems like it could produce specializations for things that we may not want them for (e.g. nsAutoPtr).  I'd rather not do that, I think.....  Justin?

> For a use-case, how about something like:

OK, sold.
Comment 7 Justin Lebar (not reading bugmail) 2011-10-21 09:33:43 PDT
> Instead of hardcoding nsCOMPtr and nsRefPtr you could use a template template parameter

I don't think that works here.  SwapToISupportsArray will take any Foo<T> class.  But we're using template specialization so that SafeElementAt(i) only exists when the T in nsTArray<T> is a pointer, nsCOMPtr, or nsRefPtr.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-10-21 09:38:38 PDT
> For a use-case, how about something like:

Actually, I don't think that works; that involves returning a non-const reference, which can't be created from a temporary.
Comment 9 Justin Lebar (not reading bugmail) 2011-10-21 11:18:59 PDT
Comment on attachment 568503 [details] [diff] [review]
Add single-argument SafeElementAt specializations for nsTArrays of smart pointers.

This looks like the only sane way to do it.  (Unless you just want to punt on the single-argument SafeElementAt for smart pointers and call the two-argument version...)
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-10-25 09:59:43 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/058b227d9a07
Comment 11 Ed Morley [:emorley] 2011-10-25 18:02:26 PDT
https://hg.mozilla.org/mozilla-central/rev/058b227d9a07

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