Closed Bug 1271751 Opened 3 years ago Closed 3 years ago

use C++11 move construction where appropriate with nsTArray_CopyWithConstructors

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(9 files)

6.51 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
15.48 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
9.71 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.96 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.77 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.66 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.09 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.05 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.30 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
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.
Maybe just:

  CopyOverlappingRegions
  MoveOverlappingRegions
  CopyNonOverlappingRegions
  MoveNonOverlappingRegions
Assignee: nobody → nfroyd
They are unused, except for tests.
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.
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.
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.
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.
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.
This change eliminates the last use of the Copy* family of functions.
After all the previous patches, we never call these functions.  Any
copying required by nsTArray is taken care of by other means.
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.
erahm approved these patches in bug 1199740.
Attachment #8769106 - Flags: review+
Attachment #8769107 - Flags: review+
Attachment #8769108 - Flags: review+
Attachment #8769109 - Flags: review+
Attachment #8769110 - Flags: review+
Attachment #8769111 - Flags: review+
Attachment #8769112 - Flags: review+
Attachment #8769113 - Flags: review+
Attachment #8769114 - Flags: review+
Pushed by nfroyd@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.