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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPCOM
--
enhancement
RESOLVED FIXED
12 years ago
23 days ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
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>
(Assignee)

Comment 1

12 years ago
We should also make sure this works with nsString objects:

  nsTArray<nsString>

Comment 2

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

Comment 3

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

Comment 4

12 years ago
(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

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

I was probably wrong. Benjamin might be correct here.
(Assignee)

Comment 6

12 years ago
Created attachment 203315 [details]
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

12 years ago
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.
(Assignee)

Comment 8

12 years ago
> 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 :-)
string's Find is in the #ifdef DEPRECATED ifdef...
(Assignee)

Comment 10

12 years ago
> 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 :-)
maybe, but then it's kinda redundant with FindCharInReadable/FindInReadable...
(Assignee)

Comment 12

12 years ago
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 ;-)
(Assignee)

Comment 13

12 years ago
Created attachment 203464 [details] [diff] [review]
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>>
Attachment #203315 - Attachment is obsolete: true
(Assignee)

Comment 14

12 years ago
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.
STL-using code tends not to compile when trying to have a container containing std::auto_ptrs.
(Assignee)

Comment 16

12 years ago
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?
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

12 years ago
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.
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.
(Assignee)

Comment 20

12 years ago
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).
so what will happen if I do:
  nsAutoPtr<nsFoo> f = ary[i];
(Assignee)

Comment 22

12 years ago
> 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...
(Assignee)

Comment 23

12 years ago
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.
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 24

12 years ago
Created attachment 203868 [details] [diff] [review]
v1 patch
Attachment #203464 - Attachment is obsolete: true
Attachment #203868 - Flags: superreview?(dbaron)
Attachment #203868 - Flags: review?(brettw)
(Assignee)

Updated

12 years ago
Attachment #203868 - Flags: review?(benjamin)

Comment 25

12 years ago
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

12 years ago
> >+    // 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.
(Assignee)

Comment 27

12 years ago
> >+    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!
(Assignee)

Comment 28

12 years ago
Created attachment 203961 [details] [diff] [review]
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
Attachment #203868 - Attachment is obsolete: true
Attachment #203961 - Flags: superreview?(dbaron)
Attachment #203961 - Flags: review?(brettw)
Attachment #203868 - Flags: superreview?(dbaron)
Attachment #203868 - Flags: review?(brettw)
Attachment #203868 - Flags: review?(benjamin)
(Assignee)

Updated

12 years ago
Attachment #203961 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #203961 - Flags: review?(benjamin) → review+
(Assignee)

Comment 29

12 years ago
Created attachment 204007 [details] [diff] [review]
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.
Attachment #203961 - Attachment is obsolete: true
Attachment #204007 - Flags: superreview?(dbaron)
Attachment #204007 - Flags: review?(brettw)
Attachment #203961 - Flags: superreview?(dbaron)
Attachment #203961 - Flags: review?(brettw)
(Assignee)

Comment 30

12 years ago
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 ;-)
Attachment #204007 - Flags: superreview?(dbaron) → superreview?(bryner)

Updated

12 years ago
Attachment #204007 - Flags: review?(brettw) → review+
Attachment #204007 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 31

12 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

12 years ago
Created attachment 204086 [details] [diff] [review]
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

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

r+sr=jag
Attachment #204086 - Flags: superreview+
Attachment #204086 - Flags: review+
(Assignee)

Comment 34

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

This patch is now in on the trunk.
Attachment #204086 - Attachment description: patch: disable nsTArray<nsAutoPtr> → patch: disable nsTArray<nsAutoPtr> [fixed-on-trunk]

Comment 35

12 years ago
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.
(Assignee)

Comment 36

12 years ago
> 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

12 years ago
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

12 years ago
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
(Assignee)

Comment 39

12 years ago
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.
Attachment #204086 - Attachment description: patch: disable nsTArray<nsAutoPtr> [fixed-on-trunk] → patch: disable nsTArray<nsAutoPtr>

Comment 40

12 years ago
Bah, stupid compiler.

Comment 41

12 years ago
Created attachment 204108 [details] [diff] [review]
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

12 years ago
> 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

12 years ago
(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

12 years ago
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

12 years ago
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*.
Attachment #204108 - Flags: review?(dbaron)
Attachment #204108 - Flags: review?(dbaron) → review+
Attachment #204108 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Attachment #204108 - Flags: review?(darin) → review+

Comment 46

12 years ago
Checking in nsAutoPtr.h;
/cvsroot/mozilla/xpcom/base/nsAutoPtr.h,v  <--  nsAutoPtr.h
new revision: 1.15; previous revision: 1.14
done
Blocks: 324274
checked in on branch, along with all followup changes to nsTArray
Keywords: fixed1.8.1

Updated

12 years ago
Summary: Provide templatized array class [down with nsVoidArray!] → Provide templatized array class, nsTArray [down with nsVoidArray!]
Blocks: 1433584
You need to log in before you can comment on or make changes to this bug.