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..337033888863b7323e2343323cf5b168849df1f3 100644
--- a/lib/typed_buffers.dart
+++ b/lib/typed_buffers.dart
@@ -65,13 +65,56 @@ abstract class _TypedDataBuffer<E> extends ListBase<E> {
     _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.
+  // We override the default implementation of `add` because it grows the list
+  // 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) {
-    for (E value in values) _add(value);
+  /// This adds values from [start] (inclusive) to [end] (exclusive) in

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

Start with single sentence. 

/// Add elements of [values] at the end of this buffer.

Don't use "This adds", just "Adds".

///
/// Adds the values from [start] (inclusive) to [end] (exclusive) of
/// [values] after all existing elements of this buffer.

Don't start sentence with lower-case letter, even inside brackets: [start]. It's
not necessary to say that the default is 0 when it's the default value. Just:

/// If [end] is omitted, it defaults to adding all elements of 
/// [values] after [start].

[nweiz, 2015/10/20 22:54:58]

This is the usual rule, but AFAIK the goal of that rule is to ensure that the
generated documentation has a meaningful single-sentence summary. Because
dartdoc appends doc comments on overridden methods to the original doc comment,
this will appear after the base [List.addAll] documentation, where a paragraph
will read better.
This is also a good guideline for non-overridden methods, but I think given this
will be a body paragraph it should use a full sentence.
Done in this case, but I don't think this is actually in the doc comment
guidelines, and enforcing it seems like it would lead to awkward sentence
structure in some cases.

[Lasse Reichstein Nielsen, 2015/10/21 10:11:18]

It does WHAT? That's horribly broken, and none of our comments have been written
to be compatible with that.

We have comments that explicitly state "subclasses should override this method".
That's not something that should be inherited.
Comments should be sentences. Sentences starts with a capital letter.  :)
I know it's not in the guide, this is just me reviewing - I find "sentences"
that start with a lower-case letter to be confusing, I always wonder if
something is missing in front of it.

[nweiz, 2015/10/21 21:22:30]

Doing an experiment, it looks like I'm wrong. Maybe I'm thinking of outdated
docgen behavior?
I've talked to Kathy and she agrees. Done.

+  /// [values]. [start] defaults to `0` and [end] defaults to `values.length`.
+  void addAll(Iterable<E> values, [int start = 0, int end]) =>

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

Don't use => for void functions. Make it a proper { } body without a return.

[nweiz, 2015/10/20 22:54:58]

Done.

+      insertAll(_length, values, start, end);
+
+  /// This adds values from [start] (inclusive) to [end] (exclusive) in
+  /// [values]. [start] defaults to `0` and [end] defaults to `values.length`.
+  void insertAll(int index, Iterable<E> values, [int start = 0, int end]) {
+    RangeError.checkValidIndex(index, this, "index", _length + 1);
+    RangeError.checkNotNegative(start);

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

...checkNotNegative(start, "start");

[nweiz, 2015/10/20 22:54:58]

Done.

+    if (end != null && start > end) {
+      throw new RangeError.range(start, 0, end, "start");

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

I'd make this an error of "end":
  throw new RangeError.range(end, start, null, "end");

[nweiz, 2015/10/20 22:54:58]

Done.

+    }
+
+    // If [values] is an iterable, we can't efficiently get its length. If we
+    // also don't have [end], we don't know how large to expand [_buffer] so we
+    // fall back to the default implementation.
+    if (end == null && values is! List) {

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

Just do this if values is not a list.
You don't actually know that values has (end-start) elements.

[floitsch, 2015/10/20 22:30:46]

I don't understand the comment.

+      var i = 0;
+      for (var value in values) {
+        if (i >= start) {
+          if (i == end) break;

[floitsch, 2015/10/20 22:30:47]

This is in the branch where "end" == null. So this can never be true.

[nweiz, 2015/10/20 22:54:58]

Done.

+          insert(index + i - start, value);

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

This is really horrible - maybe I should do something about it. :)

[floitsch, 2015/10/20 22:30:46]

I'm not sure what you mean, but we can't just insert elements one by one into
the middle of a list.

[nweiz, 2015/10/20 22:54:58]

It seems troublesome to me as well, but Lasse doesn't like eagerly converting
the iterable to a list. Can you two figure it out in person and come back to me
with an answer?

+        }
+        i++;
+      }
+      if (i < start) throw new StateError("Too few elements");

[floitsch, 2015/10/20 22:30:47]

This looks like we are checking that the iterable has 'start' elements, but we
don't check that it has 'end' elements.
Either we don't check either, or we check both.
I would prefer if we check both (contrary to 'take' and 'skip').

[nweiz, 2015/10/20 22:54:58]

In this block, we know [end] is null. Below, we rely on the [List.setRange] call
to do bounds-checking.

+      return;
+    }
+
+    if (values is List) {
+      end ??= values.length;
+      if (start > values.length || end > values.length) {
+        throw new StateError("Too few elements");

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

Don't throw here, the input is an iterable.
If start > values.length, just return.
If end > values.length, cap it to values.length.

[floitsch, 2015/10/20 22:30:47]

As stated above: I would actually prefer if we check here.
Skip and Take are special, and if users really want to be more flexible they
should go through 'skip' and 'take'.
(unless you can show me more of these flexible cases in the iterable/list
functions).

+      }
+    }
+
+    var valuesLength = end - start;
+    var newLength = _length + valuesLength;
+    _ensureCapacity(newLength);
+
+    _buffer.setRange(
+        index + valuesLength, _length + valuesLength, _buffer, index);
+    _buffer.setRange(index, index + valuesLength, values, start);
+    _length = newLength;
   }
 
   void insert(int index, E element) {
@@ -92,18 +135,28 @@ abstract class _TypedDataBuffer<E> extends ListBase<E> {
     _buffer = newBuffer;
   }
 
+  /// Ensures that [_buffer] is at least [requiredCapacity] long,
+  ///
+  /// Grows the buffer if necessary, preserving existing data.
+  void _ensureCapacity(int requiredCapacity) {
+    if (requiredCapacity < _buffer.length) return;

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

<=

[nweiz, 2015/10/20 22:54:58]

Done.

+    var newBuffer = _createBiggerBuffer(null);

[Lasse Reichstein Nielsen, 2015/10/20 08:09:49]

pass requiredCapacity as parameter?

[nweiz, 2015/10/20 22:54:58]

Done.

+    newBuffer.setRange(0, _length, _buffer);
+    _buffer = newBuffer;
+  }
+
   /**
    * Create a bigger buffer.
    *
    * This method determines how much bigger a bigger buffer should
-   * be. If [requiredLength] is not null, it will be at least that
+   * be. If [requiredCapacity] is not null, it will be at least that
    * size. It will always have at least have double the capacity of
    * the current buffer.
    */
-  List<E> _createBiggerBuffer(int requiredLength) {
+  List<E> _createBiggerBuffer(int requiredCapacity) {
     int newLength = _buffer.length * 2;
-    if (requiredLength != null && newLength < requiredLength) {
-      newLength = requiredLength;
+    if (requiredCapacity != null && newLength < requiredCapacity) {
+      newLength = requiredCapacity;
     } else if (newLength < INITIAL_LENGTH) {
       newLength = INITIAL_LENGTH;
     }
@@ -116,6 +169,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, skipCount);
+  }
+
+  /// Like [setRange], but with no bounds checking.
+  void _setRange(int start, int end, Iterable<E> source, int skipCount) {
     if (source is _TypedDataBuffer<E>) {
       _buffer.setRange(start, end, source._buffer, skipCount);
     } else {