1185763 - Rename deceptively named nsTArray::MoveElementsFrom

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Biru [:poiru]]

As seen in bug 1185759, nsTArray::MoveElementsFrom is not the best name. I think we should rename it to something like AppendRawElements. Nathan, any thoughts?

Comment 1

[Biru [:poiru]]

(In reply to Birunthan Mohanathas [:poiru] from comment #0)
> As seen in bug 1185759, nsTArray::MoveElementsFrom is not the best name. I
> think we should rename it to something like AppendRawElements. Nathan, any
> thoughts?

Gah, I meant to say AppendMovedElements. Also, I think we should remove the non-r-value reference version of MoveElementsFrom to make the intent clear.

Comment 2

[Nathan Froyd [:froydnj]]

AppendMovedElements WFM.  Is MoveElementsFrom actually doing C++-level moving currently?

Comment 3

[Biru [:poiru]]

Attachment: Part 1: Rename nsTArray::MoveElementsFrom to Merge

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #2)
> AppendMovedElements WFM.  Is MoveElementsFrom actually doing C++-level
> moving currently?

Nope. I decided to go with 'Merge' as I think that describes the intent more clearly.

Comment 4

[Biru [:poiru]]

Attachment: Part 2: Always use mozilla::Move with nsTArray::Merge

Comment 5

[Biru [:poiru]]

Attachment: Part 3: Remove non-rvalue version of nsTArray::Merge

Comment 6

[Biru [:poiru]]

Attachment: Part 4: Swap elements if possible in nsTArray::Merge

Comment 7

[Biru [:poiru]]

Attachment: Part 5: Remove now unnecessary nsTArray::Merge optimizations in parse/html/

Comment 8

[Nathan Froyd [:froydnj]]

I don't know what the right name is, but I don't think Merge is the right name, as that tends to get used for sorting algorithms...

Comment 9

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #8)
> I don't know what the right name is, but I don't think Merge is the right
> name, as that tends to get used for sorting algorithms...

How about just AppendArray, then? Together with Part 2, I think that is a sufficiently descriptive name.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #9)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #8)
> > I don't know what the right name is, but I don't think Merge is the right
> > name, as that tends to get used for sorting algorithms...
> 
> How about just AppendArray, then? Together with Part 2, I think that is a
> sufficiently descriptive name.

If we're going to do that, why not just add an infallible-only nsTArray&& overload to AppendElements?

Comment 11

[Nathan Froyd [:froydnj]]

Probably easiest to do the renaming as a part 6 followup if we go with AppendElements(nsTArray&&).

Tests would also be appreciated to make sure we get this right.

Comment 12

[Biru [:poiru]]

Attachment: Part 6: Add fallible variant of r-value nsTArray::AppendElements

I modified my local patches to use AppendElements as per comment 11.

Comment 13

[Biru [:poiru]]

Attachment: Part 7: Add test for r-value nsTArray::AppendElements

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8638009 [details] [diff] [review]
Part 7: Add test for r-value nsTArray::AppendElements

Review of attachment 8638009 [details] [diff] [review]:
-----------------------------------------------------------------

As fine as these tests are, I think it'd be better to have something like:

class MoveChecker
{
public:
  MoveChecker() { sNormalConstructs++; }
  MoveChecker(const MoveChecker&) { ...; sCopyConstructs++; }
  MoveChecker(MoveChecker&&) { ...; sMoveConstructs++; }
  ...
};

etc. similar to what test_move_array in TestTArray.cpp already does (maybe you can even reuse Countable and Movable?).  Then assert that the counts are what you expect after each operation, in addition to asserting properties of the array itself.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8638008 [details] [diff] [review]
Part 6: Add fallible variant of r-value nsTArray::AppendElements

Review of attachment 8638008 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsTArray.h
@@ +1542,1 @@
>    elem_type* AppendElements(nsTArray_Impl<Item, Allocator>&& aArray)

I forget; we want to unprotect this eventually, correct?

Comment 16

[Biru [:poiru]]

Comment on attachment 8638009 [details] [diff] [review]
Part 7: Add test for r-value nsTArray::AppendElements

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #14)
> Comment on attachment 8638009 [details] [diff] [review]
> Part 7: Add test for r-value nsTArray::AppendElements
> 
> Review of attachment 8638009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As fine as these tests are, I think it'd be better to have something like:
> 
> class MoveChecker
> {
> public:
>   MoveChecker() { sNormalConstructs++; }
>   MoveChecker(const MoveChecker&) { ...; sCopyConstructs++; }
>   MoveChecker(MoveChecker&&) { ...; sMoveConstructs++; }
>   ...
> };
> 
> etc. similar to what test_move_array in TestTArray.cpp already does (maybe
> you can even reuse Countable and Movable?).  Then assert that the counts are
> what you expect after each operation, in addition to asserting properties of
> the array itself.

With the r-value version of AppendElements, we just steal the bits of the given array rather than invoking the move constructor or move assignment operator. Therefore, I don't think the Moveable class would be appropriate here. I'm rerequesting review in hopes that you agree.

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #15)
> Comment on attachment 8638008 [details] [diff] [review]
> Part 6: Add fallible variant of r-value nsTArray::AppendElements
> 
> Review of attachment 8638008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/nsTArray.h
> @@ +1542,1 @@
> >    elem_type* AppendElements(nsTArray_Impl<Item, Allocator>&& aArray)
> 
> I forget; we want to unprotect this eventually, correct?

Yep, that'll happen after we switch (most) uses of FallibleTArray to nsTArray.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #16)
> (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #14)
> > class MoveChecker
> > {
> > public:
> >   MoveChecker() { sNormalConstructs++; }
> >   MoveChecker(const MoveChecker&) { ...; sCopyConstructs++; }
> >   MoveChecker(MoveChecker&&) { ...; sMoveConstructs++; }
> >   ...
> > };
> > 
> > etc. similar to what test_move_array in TestTArray.cpp already does (maybe
> > you can even reuse Countable and Movable?).  Then assert that the counts are
> > what you expect after each operation, in addition to asserting properties of
> > the array itself.
> 
> With the r-value version of AppendElements, we just steal the bits of the
> given array rather than invoking the move constructor or move assignment
> operator. Therefore, I don't think the Moveable class would be appropriate
> here. I'm rerequesting review in hopes that you agree.

I'm assuming that by "steal the bits", you mean "blast them into place with memcpy/memmove", correct?

That doesn't necessarily happen for all types, e.g. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#670

and looking at the code, I don't think we get this right now...

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8638009 [details] [diff] [review]
Part 7: Add test for r-value nsTArray::AppendElements

Review of attachment 8638009 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, let's fix the move issues in another bug.  They are orthogonal to the issues being addressed here.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f00f5621fca
https://hg.mozilla.org/integration/mozilla-inbound/rev/d35a4a640099
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1f72e930c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6087016fc5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b411e5efa6cf

Comment 20

[Biru [:poiru]]

Apparently I forgot to land the last 2 patches. I'll do that later after a Try push.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f00f5621fca
https://hg.mozilla.org/mozilla-central/rev/d35a4a640099
https://hg.mozilla.org/mozilla-central/rev/ca1f72e930c4
https://hg.mozilla.org/mozilla-central/rev/f6087016fc5d
https://hg.mozilla.org/mozilla-central/rev/b411e5efa6cf

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/499a2fece05b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb8086b1ffa

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/499a2fece05b
https://hg.mozilla.org/mozilla-central/rev/dcb8086b1ffa

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/499a2fece05b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dcb8086b1ffa

Comment 25

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion