Add _TypedDataBuffer.addRange.

lib/typed_buffers.dart

Index: lib/typed_buffers.dart
diff --git a/lib/typed_buffers.dart b/lib/typed_buffers.dart
index 50ed2414c4cbd58d196a720e9706ecf31338a18e..4bb6be73589bf0ccef50a847651821aae664b492 100644
--- a/lib/typed_buffers.dart
+++ b/lib/typed_buffers.dart
@@ -71,7 +71,32 @@ abstract class _TypedDataBuffer<E> extends ListBase<E> {
   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) {
@@ -116,6 +141,11 @@ abstract class _TypedDataBuffer<E> extends ListBase<E> {
 
   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 {