Add _TypedDataBuffer.addRange.

lib/typed_buffers.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Growable typed-data lists.
  *
  * These lists works just as a typed-data list, except that they are growable.
  * They use an underlying buffer, and when that buffer becomes too small, it
  * is replaced by a new buffer.
@@ skipping 52 matching lines @@
   void _add(E value) {
     if (_length == _buffer.length) _grow();
     _buffer[_length++] = value;
   }
 
   // We override the default implementation of `add` and `addAll` because
   // they grow by setting the length in increments of one. We want to grow
   // by doubling capacity in most cases.
   void add(E value) { _add(value); }
 
   void addAll(Iterable<E> values) {

[Lasse Reichstein Nielsen, 2015/10/14 07:09:59]

Maybe addAll is a special case of insertAll, and we should really do
insertAll(int index, Iterable<E> values, [int start = 0, int end])
and just have addAll delegate to insertAll(_length, ...).

[Lasse Reichstein Nielsen, 2015/10/14 07:10:00]

I'd rather have specialized list code and general iterable code instead of
converting an iterable to a list. Unnecessary copying is a waste of resources.

void addAll(Iterable<E> values, [int start = 0, int end]) {
  RangeError.checkNotNegative(start, "start");
  if (end != null) RangeError.checkValidRange(end, start, null, "end");
  if (values is List) {
    ... handle list ...
  } else {
    _addAllIterable(values, start, end); 
  }
}

[nweiz, 2015/10/14 20:20:20]

Really? This is contrary to what ListBase does in most cases.

[nweiz, 2015/10/14 20:20:20]

Done. ListBase.insertAll bottoms out on setRange anyway for Lists, so this works
out nicely.

Do you know why ListBase.addAll doesn't have the same sort of setRange logic?

-    for (E value in values) _add(value);
+    var list = values is List ? values : values.toList();
+    var newLength = _length + list.length;
+    _createBiggerBuffer(newLength);

[Lasse Reichstein Nielsen, 2015/10/14 07:10:00]

Only grow the backing store if necessary.
The _createBiggerBuffer function does not grow anything, it just allocates and
returns a new buffer that you have to fill and install manually.
  
  if (_buffer.length < newLength) {
    _buffer = _createBiggerBuffer(newLength)
                  ..setRange(0, _length, _buffer); 
  }

We could create an _ensureCapacity(int newLength) function that does the above.

[nweiz, 2015/10/14 20:20:20]

Done.

+    _setRange(_length, newLength, list);
+    _length = newLength;
+  }
+
+  /// Adds a range of values in [source] to the end of the buffer.

[Lasse Reichstein Nielsen, 2015/10/14 07:09:59]

Comment format is inconsistent with the rest of the file/library.

[nweiz, 2015/10/14 20:20:20]

The existing style is already inconsistent—see [_buffer]/[_library]. I can do a
follow-up CL to normalize it and make it follow the style guide.

+  ///
+  /// This adds values from [sourceStart] (inclusive) to [sourceEnd]
+  /// (exclusive). If [sourceEnd] is omitted, adds all values after
+  /// [sourceStart].
+  void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) {

[Lasse Reichstein Nielsen, 2015/10/14 07:09:59]

Validate sourceStart and sourceEnd:
  0 <= sourceStart (<= sourceEnd if not null)

[Lasse Reichstein Nielsen, 2015/10/14 07:09:59]

Just add the parameters to addAll;
  void addAll(Iterable<E> values, [int start = 0, int end])
This is the standard format we use for passing a range of something.

Lesson in library design: Java did this right - you have a cheap way to make a
slice of a list/string/etc, so you don't have to add start/end parameters to
every function that might take a list/string/etc to avoid copying the elements.

[Lasse Reichstein Nielsen, 2015/10/14 07:10:00]

Naming-wise, functions named "somethingRange" usually refer to a range on the
current list, so this one can be confusing.

[nweiz, 2015/10/14 20:20:20]

Acknowledged.

[nweiz, 2015/10/14 20:20:20]

Done.

Do you really prefer "start" and "end" to "sourceStart" and "sourceEnd"? Usually
"start" and "end" refer to indexes in [this].

[nweiz, 2015/10/14 20:20:20]

Done.

+    var list;
+    if (source is List) {
+      list = source;
+    } else {
+      if (sourceEnd != null) source = source.take(sourceEnd);
+      list = source.skip(sourceStart).toList();

[Lasse Reichstein Nielsen, 2015/10/14 07:10:00]

This looks wrong - you create a list of only the elements from sourceStart to
sourceEnd, but don't adjust sourceStart/sourceEnd.

Instead of converting the iterable to a list, I'd rather have different code for
List and non-List Iterable. Copying should be avoided if possible.

[nweiz, 2015/10/14 20:20:20]

Done.
Again, this seems contrary to the implementation of ListBase.

+    }
+
+    sourceEnd ??= list.length;
+    var newLength = _length + sourceEnd - sourceStart;
+    _createBiggerBuffer(newLength);
+    _setRange(_length, newLength, list, sourceStart);
+    _length = newLength;
   }
 
   void insert(int index, E element) {
     if (index < 0 || index > _length) {
       throw new RangeError.range(index, 0, _length);
     }
     if (_length < _buffer.length) {
       _buffer.setRange(index + 1, _length + 1, _buffer, index);
       _buffer[index] = element;
       _length++;
@@ skipping 24 matching lines @@
     }
     return _createBuffer(newLength);
   }
 
   void _grow() {
     _buffer = _createBiggerBuffer(null)..setRange(0, _length, _buffer);
   }
 
   void setRange(int start, int end, Iterable<E> source, [int skipCount = 0]) {
     if (end > _length) throw new RangeError.range(end, 0, _length);
+    _setRange(start, end, source._buffer, skipCount);
+  }
+
+  /// Like [setRange], but with no bounds checking.
+  void _setRange(int start, int end, Iterable<E> source, [int skipCount = 0]) {

[Lasse Reichstein Nielsen, 2015/10/14 07:10:00]

Make skipCount not optional here.

I generally don't use optionals on private functions, the extra `0` only get to
annoy me, and it's likely to have less overhead when calling.

[nweiz, 2015/10/14 20:20:20]

Done.

     if (source is _TypedDataBuffer<E>) {
       _buffer.setRange(start, end, source._buffer, skipCount);
     } else {
       _buffer.setRange(start, end, source, skipCount);
     }
   }
 
   // TypedData.
 
   int get elementSizeInBytes => _buffer.elementSizeInBytes;
@@ skipping 97 matching lines @@
   Int32x4 get _defaultValue => _zero;
   Int32x4List _createBuffer(int size) => new Int32x4List(size);
 }
 
 class Float32x4Buffer extends _TypedDataBuffer<Float32x4> {
   Float32x4Buffer([int initialLength = 0])
       : super(new Float32x4List(initialLength));
   Float32x4 get _defaultValue => new Float32x4.zero();
   Float32x4List _createBuffer(int size) => new Float32x4List(size);
 }