Last Comment Bug 686597 - nsAutoTArray doesn't have a copy-constructor.
: nsAutoTArray doesn't have a copy-constructor.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Justin Lebar (not reading bugmail)
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on: 677571
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 17:27 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-09-16 07:30 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a copy constructor to nsAutoTArray and friends. (1.78 KB, patch)
2011-09-14 16:53 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review
Patch v2 (1.67 KB, patch)
2011-09-15 20:17 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-09-13 17:27:49 PDT
In bug 682735 comment 52, we noticed that nsAutoTArray doesn't have a copy-constructor.  The following code:

  nsAutoTArray<E, 16> foo;
  // put some elements in foo
  nsAutoTArray<E, 16> bar(foo);

effectively makes |bar| a nsTArray; |bar| will never use its auto storage.

We can fix this simply by adding copy constructors.
Comment 1 Justin Lebar (not reading bugmail) 2011-09-14 14:36:54 PDT
Luke pointed out that js vectors don't have copy constructors -- if you want to copy a vector, you need to do so explicitly.

I'm going to see whether we can just do that here.
Comment 2 Justin Lebar (not reading bugmail) 2011-09-14 14:46:51 PDT
Well, nsTArray has a copy-constructor, and I'm not sure we're ready to get rid of that.  So maybe nsAutoTArray should also have one.
Comment 3 Justin Lebar (not reading bugmail) 2011-09-14 16:53:20 PDT
Created attachment 560281 [details] [diff] [review]
Add a copy constructor to nsAutoTArray and friends.

I need to test that this fixes the issues we saw in bug 682735 comment 52, but unfortunately my instrumentation doesn't work on mac.  I think this should work...
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-09-14 22:51:29 PDT
Comment on attachment 560281 [details] [diff] [review]
Add a copy constructor to nsAutoTArray and friends.

>+++ b/xpcom/glue/nsTArray.h
>+  nsAutoArrayBase(const nsAutoArrayBase<TArrayBase, N> &aOther) {

Any reason to not make that argument |const TArrayBase &aOther|?

r=me either way.
Comment 5 Justin Lebar (not reading bugmail) 2011-09-15 12:00:24 PDT
(In reply to Boris Zbarsky (:bz) from comment #4)
> >+++ b/xpcom/glue/nsTArray.h
> >+  nsAutoArrayBase(const nsAutoArrayBase<TArrayBase, N> &aOther) {
> 
> Any reason to not make that argument |const TArrayBase &aOther|?

I think just to emphasize that the constructor shouldn't be called except by nsAutoArrayBase's children's copy-constructors.

But in that case, maybe it should be protected...
Comment 6 Justin Lebar (not reading bugmail) 2011-09-15 20:17:16 PDT
Created attachment 560504 [details] [diff] [review]
Patch v2

Making the constructors protected.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-09-15 21:00:32 PDT
Comment on attachment 560504 [details] [diff] [review]
Patch v2

r=me

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