Closed Bug 976927 Opened 11 years ago Closed 10 years ago

Support operator= from nsTArray to nsAutoTArray

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(3 files, 1 obsolete file)

Classes don't inherit operator=, so each class needs to provide.
gcc says: operatorequals.cpp: In function 'int main()': operatorequals.cpp:25:8: error: no match for 'operator=' in 'd2 = b' operatorequals.cpp:25:8: note: candidate is: operatorequals.cpp:17:8: note: D2& D2::operator=(const D2&) operatorequals.cpp:17:8: note: no known conversion for argument 1 from 'B' to 'const D2&
nsTArray_Impl::operator const nsTArray<E>&() const provides support for assigning from nsAutoTArray to nsTArray.
Blocks: 857610
Summary: Support operator= between nsTArray and nsAutoTArray → Support operator= from nsTArray to nsAutoTArray
Version: 26 Branch → Trunk
Attached patch patch (obsolete) (deleted) — Splinter Review
This extends what I assume http://hg.mozilla.org/mozilla-central/diff/1a0cd3aa1864/xpcom/glue/nsTArray.h#l1.535 was trying to achieve. Also remove unnecessary nsTArray_Impl operator= template, because template<typename Allocator> nsTArray_Impl::operator const nsTArray_Impl<E, Allocator>&() const provides support for assigning nsTArray_Impls of different Allocators.
Attachment #8381957 - Flags: review?(benjamin)
Comment on attachment 8381957 [details] [diff] [review] patch Is this urgent? This will conflict with bug 968520 which will significantly change how this works, including the allocator bits.
Flags: needinfo?(karlt)
No, this isn't urgent. I can easily work around the lack of operator= by using Base::operator= explicitly, but perhaps that might also conflict with bug 968520.
Flags: needinfo?(karlt)
Comment on attachment 8381957 [details] [diff] [review] patch Going to cancel review here and if appropriate please ask froydnj.
Attachment #8381957 - Flags: review?(benjamin)
Attachment #8381957 - Flags: review?(nfroyd)
Comment on attachment 8381957 [details] [diff] [review] patch Review of attachment 8381957 [details] [diff] [review]: ----------------------------------------------------------------- Assuming the answer to the question below is "yes", r=me. I had a really hard time reading the commit message, I thought you meant (mistyped) |operator=| rather than |operator()| and was very confused! This patch will require some rebasing over XPCOM formatting changes. ::: xpcom/glue/nsTArray.h @@ -840,5 @@ > - template<typename Allocator> > - self_type& operator=(const nsTArray_Impl<E, Allocator>& other) { > - ReplaceElementsAt(0, Length(), other.Elements(), other.Length()); > - return *this; > - } Ah, so we don't need this because the compiler will use the templated operator() to convert to the proper type, and then call operator=(const self_type&)?
Attachment #8381957 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #7) > Ah, so we don't need this because the compiler will use the templated > operator() to convert to the proper type, and then call operator=(const > self_type&)? Yes. It's not precisely the same interface because only one conversion may be performed when passing a parameter, so if anything else converts to an nsTArray_Impl of only one possible Allocator, then no subsequent conversion to another Allocator is possible. However nsTArray_Impl operator= is an internal interface, so I don't think this is an issue. > I had a really > hard time reading the commit message, I thought you meant (mistyped) > |operator=| rather than |operator()| and was very confused! Yes, sorry, that was a bit cryptic. I've updated the comment in an attempt to clarify: and remove an unnecessary nsTArray_Impl operator= template, because the conversion operator template<typename Allocator> nsTArray_Impl::operator const nsTArray_Impl<E, Allocator>&() const will convert nsTArray_Impls of different Allocators when passed to nsTArray_Impl operator=(const self_type& aOther).
Now that nsTArray_Impl has self_type& operator=(self_type&& aOther), removing the templated operator= leads to ambiguous choice of operator= after conversion, so I'll leave nsTArray_Impl as is and only add the methods to the Auto classes. In file included from /mnt/ssd1/karl/moz/dev/dom/svg/SVGStringList.h:10:0, from ../../dist/include/mozilla/dom/SVGTests.h:10, from ../../dist/include/mozilla/dom/SVGGraphicsElement.h:9, from ../../dist/include/mozilla/dom/SVGTextContentElement.h:9, from ../../dist/include/mozilla/dom/SVGTextPositioningElement.h:9, from ../../dist/include/mozilla/dom/SVGTextElement.h:9, from /mnt/ssd1/karl/moz/dev/dom/svg/SVGTextElement.cpp:6, from /mnt/sda11/karl/obj/dom/svg/Unified_cpp_dom_svg7.cpp:2: ../../dist/include/nsTArray.h: In instantiation of 'FallibleTArray<E>::self_type& FallibleTArray<E>::operator=(const nsTArray_Impl<E, Allocator>&) [with Allocator = nsTArrayInfallibleAllocator; E = mozilla::nsSVGTransform; FallibleTArray<E>::self_type = FallibleTArray<mozilla::nsSVGTransform>]': /mnt/ssd1/karl/moz/dev/dom/svg/SVGTransformList.cpp:50:12: required from here ../../dist/include/nsTArray.h:1853:5: error: call of overloaded 'operator=(const nsTArray_Impl<mozilla::nsSVGTransform, nsTArrayInfallibleAllocator>&)' is ambiguous ../../dist/include/nsTArray.h:1853:5: note: candidates are: ../../dist/include/nsTArray.h:841:14: note: nsTArray_Impl<E, Alloc>::self_type& nsTArray_Impl<E, Alloc>::operator=(const self_type&) [with E = mozilla::nsSVGTransform; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::self_type = nsTArray_Impl<mozilla::nsSVGTransform, nsTArrayFallibleAllocator>] ../../dist/include/nsTArray.h:850:14: note: nsTArray_Impl<E, Alloc>::self_type& nsTArray_Impl<E, Alloc>::operator=(nsTArray_Impl<E, Alloc>::self_type&&) [with E = mozilla::nsSVGTransform; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::self_type = nsTArray_Impl<mozilla::nsSVGTransform, nsTArrayFallibleAllocator>]
Rebased.
Attachment #8525579 - Flags: review?(nfroyd)
Attachment #8381957 - Attachment is obsolete: true
Comment on attachment 8525579 [details] [diff] [review] use nsAutoTArray operator= from nsTArray Review of attachment 8525579 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webaudio/DelayBuffer.cpp @@ +246,5 @@ > NS_WARN_IF_FALSE(mHaveWrittenBlock || aNewReadChunk != mCurrentChunk, > "Smoothing is making feedback delay too small."); > > mLastReadChunk = aNewReadChunk; > + mUpmixChannels = mChunks[aNewReadChunk].mChannelData; So this is not quite the same thing, right? Before, we were only copying in elements of mChunks[aNewReadChunk].mChannelData and either leaving elements in mUpmixChannels the same, or not copying all of mChannelData. I think you'll have to ask a webaudio person to look at this; I don't know whether this change is correct.
Attachment #8525579 - Flags: review?(nfroyd)
Comment on attachment 8525579 [details] [diff] [review] use nsAutoTArray operator= from nsTArray >- mUpmixChannels.ReplaceElementsAt(0, mUpmixChannels.Length(), >- mChunks[aNewReadChunk].mChannelData); >+ mUpmixChannels = mChunks[aNewReadChunk].mChannelData; (In reply to Nathan Froyd (:froydnj) from comment #12) > So this is not quite the same thing, right? Before, we were only copying in > elements of mChunks[aNewReadChunk].mChannelData and either leaving elements > in mUpmixChannels the same, or not copying all of mChannelData. They are meant to be the same thing. It looks like the same code to me. http://hg.mozilla.org/mozilla-central/annotate/aa72ddfe9f93/xpcom/glue/nsTArray.h#l841 http://hg.mozilla.org/mozilla-central/annotate/aa72ddfe9f93/xpcom/glue/nsTArray.h#l1181
Attachment #8525579 - Flags: review?(padenot)
Comment on attachment 8525579 [details] [diff] [review] use nsAutoTArray operator= from nsTArray Review of attachment 8525579 [details] [diff] [review]: ----------------------------------------------------------------- Oh. Yes, you're correct.
Attachment #8525579 - Flags: review?(padenot) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: