316782 - Provide templatized array class, nsTArray [down with nsVoidArray!]

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Darin Fisher]

Provide templatized array class [down with nsVoidArray!]

The following should just work:

  nsTArray<PRBool>
  nsTArray<PRInt64>
  nsTArray<nsFoo>
  
And this:

  typedef nsAutoPtr<nsBar> nsAutoBar;
  nsTArray<nsAutoBar>

Comment 1

[Darin Fisher]

We should also make sure this works with nsString objects:

  nsTArray<nsString>

Comment 2

[Benjamin Smedberg]

Would nsTArray<PRBool> hold pointers-to-PRBool or just PRBool?

Comment 3

[Benjamin Smedberg]

For example, could nsCOMArray<t> just be nsTArray< nsCOMPtr<T> > ?

Comment 4

[Brett Wilson]

(In reply to comment #3)
> For example, could nsCOMArray<t> just be nsTArray< nsCOMPtr<T> > ?

I don't think so. COM Array does things like addref when an object is read, and requires a more limited API. I think we need something very much like std::vector, so nsTArray<PRBool> would hold an array of bools and NOT pointers, and would return references from most operations. Then you could can "foo[5]++;" for example.

Comment 5

[Brett Wilson]

(In reply to comment #4)
> (In reply to comment #3)
> I don't think so.

I was probably wrong. Benjamin might be correct here.

Comment 6

[Darin Fisher]

Attachment: quick prototype of nsTArray API

Comments welcome.  The API will be much better documented in the final version.  This is just an initial stab at the API for the purposes of gathering feedback.

I'm somewhat concerned with the use of member templates, but we'll see how it goes.  If it comes down to it, we could just punt and use a virtual function.

Comment 7

[Brett Wilson]

Some suggestions:

I'd like an STL-style Reserve() function that sets the size of the buffer. This way if you know about how many items you will be inserting, you can avoid reallocations. Less important might be the corresponding Capacity() (maybe rename to something ReservationSize() to avoid confusion).

Do we want Prepend* functions? I'm worried people might see these and be more inclined to use them We want to encourage people to Append since that is much more efficient for an object like this and often won't require reallocation. Prepend is the same complexity as insert, so maybe people wanting to add stuff at the beginning should just use insert.

Rename RemoveAll to Clear()? I think this matches the current array and also STL.

IndexOf would be more flexible if you could give it the index to start searching (default to 0). LastIndexOf would also take an index (defaulting to the end) to start searching backwards from. Should these be renamed to Find and RFind to match TString's interface?

I'm lukewarm on the existance of RemoveElement. The implementation is so simple it doesn't help much. And one problem the current interfaces have is out of control helper functions, and I think these should be pared down the minimum. It's also difficult for me to imagine a common use case. It seems like you often know the location of something if you want to delete it.

I like STL's generic copy and insert functions because they allow you to insert/copy more generic things and portions of other vectors. Perhaps Replace/InsertElements could instead take a pointer and a count as input? Then you would say me.InsertElements(5, &other[2], 10); to insert items 2-12. Then you could also use it if you had a "regular" array, too.

Comment 8

[Darin Fisher]

> I'd like an STL-style Reserve() function that sets the size of the buffer. 

Yeah, this is good idea, and it fits well with the constructor that does something similar.


> reallocations. Less important might be the corresponding Capacity() (maybe
> rename to something ReservationSize() to avoid confusion).

The capacity is given by AllocationLength (see nsTArray_base).


> Do we want Prepend* functions? I'm worried people might see these and be more
> inclined to use them We want to encourage people to Append since that is much
> more efficient for an object like this and often won't require reallocation.
> Prepend is the same complexity as insert, so maybe people wanting to add stuff
> at the beginning should just use insert.

That's a fair argument.  I'm happy to make that change.  I was just going to implement Prepend in terms of Insert anyways ;-)


> Rename RemoveAll to Clear()? I think this matches the current array and also
> STL.

I chose RemoveAll because it uses the same verb as the other functions, but Clear does have familiarity going for it.  OK.


> IndexOf would be more flexible if you could give it the index to start
> searching (default to 0). LastIndexOf would also take an index (defaulting to
> the end) to start searching backwards from. Should these be renamed to Find 
> and RFind to match TString's interface?

Yes, I like the start index suggestion.  I'll add that as a parameter with a default value.  It will come after the comparator I suppose :-/

I bounced back-n-forth between IndexOf and Find.  IndexOf is Javascript-esque and it is also what nsVoidArray uses.  Find and RFind are nice because they correspond to the string code and the STL.  Hmm... though Find is not defined on nsTAString (yet)!



> I'm lukewarm on the existance of RemoveElement. The implementation is so 
> simple it doesn't help much. And one problem the current interfaces have is 
> out of control helper functions, and I think these should be pared down the 
> minimum. It's also difficult for me to imagine a common use case. It seems 
> like you often know the location of something if you want to delete it.

RemoveElement(element) is extremely common in the codebase.  I too like keeping the interface minimal, but helper functions rule for minimizing complexity in the code of consumers.


> I like STL's generic copy and insert functions because they allow you to
> insert/copy more generic things and portions of other vectors. Perhaps
> Replace/InsertElements could instead take a pointer and a count as input? Then
> you would say me.InsertElements(5, &other[2], 10); to insert items 2-12. Then
> you could also use it if you had a "regular" array, too.

Oh, that's a neat idea.  Sort of like iterators :-)

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

string's Find is in the #ifdef DEPRECATED ifdef...

Comment 10

[Darin Fisher]

> string's Find is in the #ifdef DEPRECATED ifdef...

True, but note that it (or some variant on it) really should be moved up to nsTAString :-)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe, but then it's kinda redundant with FindCharInReadable/FindInReadable...

Comment 12

[Darin Fisher]

So, it turns out that "nsTArray< nsAutoPtr<Object> >" doesn't work that well because nsAutoPtr does not define a copy-constructor taking a const nsAutoPtr<T> reference.  As a result, the default copy-constructor is used, which just copies mRefPtr when adding items to the array.  That results in two nsAutoPtr objects having a reference to the same Object, which causes a crash at some later point.

One solution is to define another copy-constructor that takes a const argument, and cast away the const and call .forget() just like the non-const version.  However, dbaron pointed out that STL's auto_ptr doesn't do this.  It turns out that it (SGI's implementation at least) also has this same problem as well as others (e.g., push_back doesn't compile).  So, it seems like STL doesn't support using auto_ptr with collection classes.  I wonder if we should try to support it, or if we shouldn't bother.

nsClassHashtable doesn't have this problem because UserDataType is non-const.

I should probably post my existing nsTArray implementation so people have something to refer to in this discussion ;-)

Comment 13

[Darin Fisher]

Attachment: v0.1 patch - incomplete

This is an incomplete patch.  I still need to finish documenting and testing the API.  There is also the open issue of how to handle nsTArray<nsAutoPtr<T>>

Comment 14

[Darin Fisher]

One solution to the nsAutoPtr problem would be to provide a specialization of nsTArrayElementTraits for nsAutoPtr<T>.  That could define a Construct(x,y) method that would remove the const from y to make things work.  This provides us with a way to avoid changing nsAutoPtr.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

STL-using code tends not to compile when trying to have a container containing std::auto_ptrs.

Comment 16

[Darin Fisher]

Right, so the question is:  do we want to support it here or just punt on it?  I think we can support it easily enough via the approach mentioned in comment #14.  Thoughts?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

std::auto_ptr doesn't meet the requirements to be contained in an std::vector (since it isn't copy-constructable). I wonder if it might not be a bad idea to make the const version of the nsAutoPtr copy constructor be private and undefined to avoid the problems you were seeing.

Why do we need to store nsAutoPtrs in nsTArrays? Is there code using nsVoidArray that relies on this? Could that code be switched over to use nsRefPtr (which is the general solution to this problem with std::auto_ptr)?

Comment 18

[Benjamin Smedberg]

We have quite a lot of code that stores "new nsClass" in a nsVoidArray and then manually deletes these objects before shutdown... it would be nice to provide some destructor-safety for this fairly-common usecase.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

All auto_ptr implementations suffer from the problem (by design).  I'd say punt on it for now; even if you added a flag, e.g. DeleteOnRemove(PR_TRUE) or something to call delete on removed elements, it'll end up biting someone who wants to keep a pointer around.  Then you start having to provide RemoveWithoutDelete, or change the flag first then call Remove, etc.

Does nsRefPtr work?  If so, I think that's the solution -- if someone wants reference counting, then they need to go for it all the way.  Anything else will end up hurting.

Comment 20

[Darin Fisher]

vlad: We have a way to do "remove without delete".  It's called nsAutoPtr::forget.  Use case might look something like this:

  nsTArray<nsAutoPtr<Object> > ary;
  ...
  Object *obj = ary[i].forget();
  ary.RemoveElementAt(i);

Again, I'm happy to go either way with this.  I think we can implement nsTArray<nsAutoPtr> in a sane way, so then why shouldn't we?

nsTArray<nsRefPtr> should work (it has a defined 'const-arg' copy-constructor).

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

so what will happen if I do:
  nsAutoPtr<nsFoo> f = ary[i];

Comment 22

[Darin Fisher]

> so what will happen if I do:
>   nsAutoPtr<nsFoo> f = ary[i];

hmm... if |ary| is non-const, then it would probably transfer the pointer from ary[i] to f, leaving ary[i] with a value of null.  if |ary| is const, then it will fail to compile.  or, could it try to use nsAutoPtr's "operator T*" in conjunction with the corresponding constructor for nsAutoPtr?  that would be bad.  hmm...

Comment 23

[Darin Fisher]

I think it is probably sensible to disable support for nsTArray<nsAutoPtr> for now.  I'll do so by declaring a private copy-constructor for nsAutoPtr.

Comment 24

[Darin Fisher]

Attachment: v1 patch

Comment 25

[Brett Wilson]

Comment on attachment 203868 [details] [diff] [review]
v1 patch

>+    // We prefix mData with a structure of this type.  This is done to minimize
>+    // the size of the nsTArray object when it is empty.  mLength is not
>+    // included in this structure because it is accessed frequently enough to
>+    // warrant being a proper member variable of nsTArray.
>+    struct Header {
>+      PRUint32 mCapacity;
>+    };

Is this complication necessary? It seems like it would only be worthwhile if there are a whole bunch of 0 empty arrays sitting around, which I doubt is the case. And it makes the code a little less clear.

>+    // Invoke the default constructor in place.
>+    static inline void Construct(E *e) {
>+      new (NS_STATIC_CAST(void *, e)) E();
>+    }
>+    // Invoke the copy-constructor in place.
>+    template<class A>
>+    static inline void Construct(E *e, const A &arg) {
>+      new (NS_STATIC_CAST(void *, e)) E(arg);
>+    }
>+    // Invoke the destructor in place.
>+    static inline void Destruct(E *e) {
>+      e->~E();
>+    }

I recall that this is not legal C++, in that the constructor and destructor are not technically members of a class. The thing I remember reading said that Borland C++ is the only compiler that got this right.

>+    elem_type* Elements() {
>+      return (elem_type *) mData; 
>+    }
>+    const elem_type* Elements() const {
>+      return (const elem_type *) mData; 
>+    }

I guess the point of these functions is to save you casting all over the place. Is there a need to make them public? I'd rather read code that says &ElementAt(0) than have to go look up to verify that Elements() does what I think it does.

>+    index_type IndexOf(const elem_type& e, const Comparator& c) const {
>+      const elem_type* iter = Elements(), *end = iter + Length();
>+      for (; iter != end; ++iter) {
>+        if (c.Equals(*iter, e))
>+          return iter - Elements();
>+      }
>+      return -1;
>+    }

Shouldn't you use your handy NoIndex parameter defined above in these situations?

>+    index_type IndexOf(const elem_type& e) const {
>+    index_type LastIndexOf(const elem_type& e, const Comparator& c) const {

I thought there was going to be a starting point index after the comparator (same for find last index of).

Will continue with .cpp and testcases later...

Comment 26

[Benjamin Smedberg]

> >+    // Invoke the destructor in place.
> >+    static inline void Destruct(E *e) {
> >+      e->~E();

> I recall that this is not legal C++, in that the constructor and destructor 

No, it is legal (at least in my spec) to call destructors manually, and it is recommended/required when you're using placement-new.

Comment 27

[Darin Fisher]

> >+    struct Header {
> >+      PRUint32 mCapacity;
> >+    };
> 
> Is this complication necessary?

nsVoidArray is optimized to have only a 4-byte footprint when empty, which was probably done on account of the fact that nsVoidArray is used as a member variable by a lot of classes.  I considered making nsTArray also have only a 4-byte footprint, but I decided that there were too many advantages to having mLength as a member variable.  Also, at one point during the design of this class, I stored the element size on Header, but I realized that it was something that I could always pass as a parameter to the various nsTArray_base methods that needed it.  I can imagine adding additional information to Header if we ever wanted to support a subclass of nsTArray that has some stack-based storage (i.e., nsTAutoArray).  In that case, we'd need a bit someplace that would indicate that the storage is not heap allocated.


> I recall that this is not legal C++

Well, I patterned that code off of the SGI STL implementation, and it seems to compile fine with VC7.  I haven't tried VC6 yet ;-)

One thing about VC7 is that it does not permit things like:

   int x;
   x->~int();

But, it does allow you to invoke int's "destructor" when int is a template parameter.  Odd.


> >+    elem_type* Elements() {

> I guess the point of these functions is to save you casting all over the 
> place.  Is there a need to make them public? I'd rather read code that says
> &ElementAt(0) than have to go look up to verify that Elements() does what I
> think it does.

Elements() is handy for consumers too, especially if they have prepared an array to be passed to an XPCOM method.  While &ElementAt[0] mostly works, I just found it to be somewhat verbose when all I really want is a pointer to the elements of the array.  Also, ElementAt[0] would assert if the array was empty, whereas Elements() would return null.


> >+    index_type IndexOf(const elem_type& e, const Comparator& c) const {
...
> >+      return -1;
> 
> Shouldn't you use your handy NoIndex parameter defined above in these
> situations?

Yes.  Oversight.


> >+    index_type IndexOf(const elem_type& e) const {
> >+    index_type LastIndexOf(const elem_type& e, const Comparator& c) const {
> 
> I thought there was going to be a starting point index after the comparator
> (same for find last index of).

Yes.  Oversight.


> Will continue with .cpp and testcases later...

Thanks!

Comment 28

[Darin Fisher]

Attachment: v1.1 patch

Updated patch:

 * Added 'start' parameter to IndexOf and LastIndexOf
 * Added additional testcase for IndexOf and LastIndexOf
 * Used NoIndex in place of -1

Comment 29

[Darin Fisher]

Attachment: v1.2 patch

This version compiles on VC6.  It turns out that VC6 didn't like my QuickSortComparator function.  Apparently, static template methods declared on a template class don't work... or, at least attempting to take the address of such a static function makes VC6 unhappy.  So, I moved the static function to a new class (nsQuickSortComparator) so that I could avoid having to templatize the function itself.

Comment 30

[Darin Fisher]

Comment on attachment 204007 [details] [diff] [review]
v1.2 patch

Moving SR request to bryner since dbaron won't have time for a little while, and bryner keeps asking when this will be landed ;-)

Comment 31

[Darin Fisher]

fixed-on-trunk

Comment 32

[Darin Fisher]

Attachment: patch: disable nsTArray<nsAutoPtr>

I forgot to include this part in my previous patch.  It disables support for "nsTArray<nsAutoPtr<T> >" by declaring a private copy-ctor for nsAutoPtr.

Comment 33

[jag (Peter Annema)]

Comment on attachment 204086 [details] [diff] [review]
patch: disable nsTArray<nsAutoPtr>

r+sr=jag

Comment 34

[Darin Fisher]

Comment on attachment 204086 [details] [diff] [review]
patch: disable nsTArray<nsAutoPtr>

This patch is now in on the trunk.

Comment 35

[jag (Peter Annema)]

Actually, come to think of it, we provide a copy constructor already (with non-const parameter), which should prevent the default copy constructor from being generated. So technically, this patch wasn't necessary.

Comment 36

[Darin Fisher]

> Actually, come to think of it, we provide a copy constructor already (with
> non-const parameter), which should prevent the default copy constructor from
> being generated. So technically, this patch wasn't necessary.

It was needed by GCC 3.2.2 because without it, I ended up having the default copy-ctor being invoked, which resulted in a crash (double free).  Try it yourself.  xpcom/tests/TestTArray.cpp has a commented out testcase for nsTArray<nsAutoPtr>.

Comment 37

[jag (Peter Annema)]

Ok, so it wasn't the default copy constructor that got invoked, it was the |operator T*()| in combination with the explicit call to a (any!) constructor, in this case |nsAutoPtr(T*)|.

Providing the copy constructor that takes a const self& causes the compiler to prefer that over the implicit conversion, at which point compilation fails because it's private.

std::auto_ptr doesn't provide |operator T*()| and therefore doesn't need to provide |auto_ptr(const auto_ptr&);|.

Btw, we might consider making the constructor that takes a raw T* to be explicit, and to add |operator void*() { return get(); }| so that delete on a nsAutoPtr will fail (due to ambiguity of which automatic conversion operator to select).

I found that trick at http://www.awprofessional.com/articles/article.asp?p=31529&seqNum=7

Comment 38

[Pete Zha]

It seems this checkin cause following bustage...

txInstructions.cpp
CC -o txInstructions.o -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"SunOS5\" -DOSARCH=\"SunOS\" -DBUILD_ID=2005112410  -I../../../../dist/include/string -I../../../../dist/include/xpcom -I../../../../dist/include/dom -I../../../../dist/include/content -I../../../../dist/include/widget -I../../../../dist/include/necko -I../../../../dist/include/unicharutil -I../../../../dist/include/xpconnect -I../../../../dist/include/js -I../../../../dist/include/htmlparser -I../../../../dist/include/webshell -I../../../../dist/include/docshell -I../../../../dist/include/layout -I../../../../dist/include/uconv -I../../../../dist/include/caps -I../../../../dist/include/windowwatcher -I../../../../dist/include/mimetype -I../../../../dist/include/intl -I../../../../dist/include/locale -I../../../../dist/include/htmlparser -I../../../../dist/include/pref -I../../../../dist/include/transformiix -I../../../../dist/include -I../../../../dist/include/nspr    -I../../../../dist/sdk/include -I. -I./../base -I./../xml -I./../xpath  -I/usr/openwin/include   -KPIC  -I/usr/openwin/include -xbuiltin=%all -features=tmplife -norunpath -mt  -DNDEBUG -DTRIMMED -xO4  -I/usr/openwin/include -DMOZILLA_CLIENT -D_MOZILLA_CONFIG_H_ -DMOZILLA_VERSION=\"1.9a1\" -DMOZILLA_VERSION_U=1.9a1 -DSOLARIS=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UINT16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_SYS_BYTEORDER_H=1 -DHAVE_GETOPT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DHAVE_LIBSOCKET=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=1 -DHAVE_LSTAT64=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_LANGINFO_CODESET=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_DEFAULT_TOOLKIT=\"gtk2\" -DMOZ_WIDGET_GTK2=1 -DMOZ_ENABLE_XREMOTE=1 -DMOZ_X11=1 -DMOZ_SUITE=1 -DMOZ_BUILD_APP=suite -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_ENABLE_XFT=1 -DMOZ_ENABLE_GNOMEUI=1 -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DJS_THREADSAFE=1 -DMOZ_ACCESSIBILITY_ATK=1 -DMOZILLA_LOCALE_VERSION=\"1.9a1\" -DMOZILLA_REGION_VERSION=\"1.9a1\" -DMOZILLA_SKIN_VERSION=\"1.8\"  txInstructions.cpp
"txInstructions.cpp", line 749: Error: Formal argument aSelectExpr of type nsAutoPtr<Expr> in call to txPushNewContext::SortKey::SortKey(nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>) has an inaccessible copy constructor.
"txInstructions.cpp", line 749: Error: Formal argument aLangExpr of type nsAutoPtr<Expr> in call to txPushNewContext::SortKey::SortKey(nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>) has an inaccessible copy constructor.
"txInstructions.cpp", line 749: Error: Formal argument aDataTypeExpr of type nsAutoPtr<Expr> in call to txPushNewContext::SortKey::SortKey(nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>) has an inaccessible copy constructor.
"txInstructions.cpp", line 750: Error: Formal argument aOrderExpr of type nsAutoPtr<Expr> in call to txPushNewContext::SortKey::SortKey(nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>) has an inaccessible copy constructor.
"txInstructions.cpp", line 750: Error: Formal argument aCaseOrderExpr of type nsAutoPtr<Expr> in call to txPushNewContext::SortKey::SortKey(nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>, nsAutoPtr<Expr>) has an inaccessible copy constructor.
"txInstructions.cpp", line 749: Error: Cannot use int to initialize txPushNewContext::SortKey*.
6 Error(s) detected.
gmake[7]: *** [txInstructions.o] Error 6
gmake[7]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla/content/xslt/src/xslt'
gmake[6]: *** [libs] Error 2
gmake[6]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla/content/xslt/src'
gmake[5]: *** [libs] Error 2
gmake[5]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla/content/xslt'
gmake[4]: *** [libs] Error 2
gmake[4]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla/content'
gmake[3]: *** [libs_tier_9] Error 2
gmake[3]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla'
gmake[2]: *** [tier_9] Error 2
gmake[2]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla'
gmake[1]: *** [alldep] Error 2
gmake[1]: Leaving directory `/export/home/tinderbox/builds/seamonkey-ports/SunOS_5.10_Clobber/mozilla'
gmake: *** [alldep] Error 2

Comment 39

[Darin Fisher]

Comment on attachment 204086 [details] [diff] [review]
patch: disable nsTArray<nsAutoPtr>

OK, I backed out this change to resolve the SunOS bustage.  I'll investigate a better solution next week.

Comment 40

[jag (Peter Annema)]

Bah, stupid compiler.

Comment 41

[jag (Peter Annema)]

Attachment: A bit ugly, but it gets the job done

Pete, could you see if this compiles for you? Make sure you also pick up darin's backout of the previous patch.

Comment 42

[Pete Zha]

> Pete, could you see if this compiles for you? Make sure you also pick up
> darin's backout of the previous patch.
Yes, your patch works here. I just confirmed.

Comment 43

[Peter Müller]

(In reply to comment #42)
> > Pete, could you see if this compiles for you? Make sure you also pick up
> > darin's backout of the previous patch.
> Yes, your patch works here. I just confirmed.
> 

Here too:-) 
Solaris 9, Sun Studio 11

Comment 44

[jag (Peter Annema)]

Thanks, that's good news.

It's still an ugly solution though, but at least it is a solution (assuming Visual Studio likes this, gcc seems to).

Comment 45

[jag (Peter Annema)]

Comment on attachment 204108 [details] [diff] [review]
A bit ugly, but it gets the job done

dbaron, could you take a look at this? I personally find it rather ugly, but short of removing |operator T*() const| I don't see another way to prevent accidental copying of nsAutoPtr<T> through T*.

Comment 46

[jag (Peter Annema)]

Checking in nsAutoPtr.h;
/cvsroot/mozilla/xpcom/base/nsAutoPtr.h,v  <--  nsAutoPtr.h
new revision: 1.15; previous revision: 1.14
done

Comment 47

[Brian Ryner (not reading)]

checked in on branch, along with all followup changes to nsTArray