Closed Bug 976927 Opened 10 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) — 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>]
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+
https://hg.mozilla.org/mozilla-central/rev/b941b0bbc9bd
https://hg.mozilla.org/mozilla-central/rev/347ae075f915
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.