Closed
Bug 976927
Opened 11 years ago
Closed 10 years ago
Support operator= from nsTArray to nsAutoTArray
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: karlt, Assigned: karlt)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
Classes don't inherit operator=, so each class needs to provide.
Assignee | ||
Comment 1•11 years ago
|
||
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&
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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 6•10 years ago
|
||
Comment on attachment 8381957 [details] [diff] [review]
patch
Going to cancel review here and if appropriate please ask froydnj.
Attachment #8381957 -
Flags: review?(benjamin)
Assignee | ||
Updated•10 years ago
|
Attachment #8381957 -
Flags: review?(nfroyd)
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
(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).
Assignee | ||
Comment 9•10 years ago
|
||
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>]
Assignee | ||
Comment 10•10 years ago
|
||
Rebased.
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8525579 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8381957 -
Attachment is obsolete: true
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
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.
Description
•