1271751 - use C++11 move construction where appropriate with nsTArray_CopyWithConstructors

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

nsTArray_CopyWithConstructors has Copy and Move functions that operate according to the usages of those terms by the C mem* functions, not the usages of those terms by normal C++ code.  There are certainly places where we could be more efficient by taking advantage of move construction within nsTArray_CopyWithConstructors.  But if nothing else, cleaning up the names and using move construction would make for clearer code.

Strawman names:

  TransferOverlappingRegionsVia{Copy,Move}
  TransferNonOverlappingRegionsVia{Copy,Move}

which are quite a mouthful, but I'd like to try and reserve "Copy" for the actual C++ concept that's being invoked, so something like "CopyOverlapping..." is right out.

Comment 1

[Nathan Froyd [:froydnj]]

Maybe just:

  CopyOverlappingRegions
  MoveOverlappingRegions
  CopyNonOverlappingRegions
  MoveNonOverlappingRegions

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 0 - remove heap functions from nsTArray; r=erahm

They are unused, except for tests.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 1 - provide out-of-class definitions for some nsTArray functions; r=erahm

This change enables some of the methods in nsTArray to be lazily
instantiated, particularly the ones that care about whether the element
type is copyable.  Since we have a number of places where nsTArray is
used with move-only types, we need to ensure that unless methods
requiring copyability are actually called, those methods are not
instantiated.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 2 - rename {Copy,Move}Elements to something more accurate; r=erahm

The names {Copy,Move}Elements are based on the use of mem{cpy,move},
respectively.  However, I submit that we really want the names to
reflect the C++ operations being done, rather than the underlying
implementation details.  So let's rename these to reflect that we are
always copying the elements, and discriminate between the two cases
based on whether the regions being copied overlap or not.

Bring CopyHeaderAndElements along for the ride, as well.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add Move{Non,}OverlappingRegion; r=erahm

We'll need these for future patches as we transition nsTArray to use
moves for most of its operations rather than copies.  The implementation
of these functions are essentially cut-and-paste versions of the Copy*
functions, but using moves.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 4 - rename CopyNonOverlappingRegionWithHeader to MoveNonOverlappingRegionWithHeader; r=erahm

Whenever we're copying the header, we can be guaranteed that we're never
going to use the elements from the old array afterward, so can move (in
the C++ sense) the elements rather than copying them.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 5 - use MoveNonOverlappingRegion instead of CopyNonOverlappingRegion; r=erahm

In all of the calls to CopyNonOverlappingRegion from within nsTArray, we
don't care about the contents of the source afterwards.  So we can use
moves instead of copies to potentially make things more efficient.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 6 - make ShiftData actually move its elements rather than copying; r=erahm

This change eliminates the last use of the Copy* family of functions.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 7 - remove nsTArray_CopyWith*::Copy*; r=erahm

After all the previous patches, we never call these functions.  Any
copying required by nsTArray is taken care of by other means.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 8 - fix bug in nsTArray_CopyWithConstructors; r=erahm

The backwards copying case in MoveOverlappingRegion had a bug: rather
than destroying each element from the source range as we moved it, we
would always destroy the element at the beginning of the source range.
Fortunately, none of the existing types that were copied via
constructors seem to trigger the problematic code.

Comment 11

[Nathan Froyd [:froydnj]]

erahm approved these patches in bug 1199740.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e0ce158c0b
part 0 - remove heap functions from nsTArray; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/910cbc036dce
part 1 - provide out-of-class definitions for some nsTArray functions; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/7247f68ba110
part 2 - rename {Copy,Move}Elements to something more accurate; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c726b0821954
part 3 - add Move{Non,}OverlappingRegion; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a458e6f337ce
part 4 - rename CopyNonOverlappingRegionWithHeader to MoveNonOverlappingRegionWithHeader; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/862be98e1606
part 5 - use MoveNonOverlappingRegion instead of CopyNonOverlappingRegion; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ec59ebd897
part 6 - make ShiftData actually move its elements rather than copying; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/acf15b7ae545
part 7 - remove nsTArray_CopyWith*::Copy*; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ee49f5bd4a
part 8 - fix bug in nsTArray_CopyWithConstructors; r=erahm

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/55e0ce158c0b
https://hg.mozilla.org/mozilla-central/rev/910cbc036dce
https://hg.mozilla.org/mozilla-central/rev/7247f68ba110
https://hg.mozilla.org/mozilla-central/rev/c726b0821954
https://hg.mozilla.org/mozilla-central/rev/a458e6f337ce
https://hg.mozilla.org/mozilla-central/rev/862be98e1606
https://hg.mozilla.org/mozilla-central/rev/a4ec59ebd897
https://hg.mozilla.org/mozilla-central/rev/acf15b7ae545
https://hg.mozilla.org/mozilla-central/rev/04ee49f5bd4a