|
|
Created:
5 years, 2 months ago by nweiz Modified:
5 years, 1 month ago CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/typed_data@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd _TypedDataBuffer.addRange.
This is useful for concatenating ranges of existing lists, as comes up
occasionally when implementing converters.
Also make _TypedDataBuffer.addAll more efficient for lists.
R=lrn@google.com
Committed: https://github.com/dart-lang/typed_data/commit/4a1eb49d4f2576044d71f2dd6c3f8e619f6678a9
Patch Set 1 #
Total comments: 18
Patch Set 2 : Code review changes #
Total comments: 20
Patch Set 3 : Code review changes #
Total comments: 27
Patch Set 4 : Code review changes #Patch Set 5 : Code review changes #
Total comments: 12
Patch Set 6 : Code review changes #
Total comments: 9
Patch Set 7 : Code review changes #
Messages
Total messages: 28 (1 generated)
https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:73: void addAll(Iterable<E> values) { 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); } } https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:73: void addAll(Iterable<E> values) { 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, ...). https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:76: _createBiggerBuffer(newLength); 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. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:81: /// Adds a range of values in [source] to the end of the buffer. Comment format is inconsistent with the rest of the file/library. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { Validate sourceStart and sourceEnd: 0 <= sourceStart (<= sourceEnd if not null) https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { 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. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { Naming-wise, functions named "somethingRange" usually refer to a range on the current list, so this one can be confusing. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:92: list = source.skip(sourceStart).toList(); 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. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:148: void _setRange(int start, int end, Iterable<E> source, [int skipCount = 0]) { 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.
Code review changes
I added some tests, which I guess I forgot last time. Given that the typed buffers now share API surface that's not just from List, is it worth making _TypedDataBuffer public? https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:73: void addAll(Iterable<E> values) { On 2015/10/14 07:09:59, Lasse Reichstein Nielsen wrote: > 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, ...). 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? https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:73: void addAll(Iterable<E> values) { On 2015/10/14 07:10:00, Lasse Reichstein Nielsen wrote: > 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); > } > } Really? This is contrary to what ListBase does in most cases. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:76: _createBiggerBuffer(newLength); On 2015/10/14 07:10:00, Lasse Reichstein Nielsen wrote: > 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. Done. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:81: /// Adds a range of values in [source] to the end of the buffer. On 2015/10/14 07:09:59, Lasse Reichstein Nielsen wrote: > Comment format is inconsistent with the rest of the file/library. 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. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { On 2015/10/14 07:09:59, Lasse Reichstein Nielsen wrote: > Validate sourceStart and sourceEnd: > 0 <= sourceStart (<= sourceEnd if not null) Done. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { On 2015/10/14 07:10:00, Lasse Reichstein Nielsen wrote: > Naming-wise, functions named "somethingRange" usually refer to a range on the > current list, so this one can be confusing. Acknowledged. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:86: void addRange(Iterable<E> source, int sourceStart, [int sourceEnd]) { On 2015/10/14 07:09:59, Lasse Reichstein Nielsen wrote: > 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. Done. Do you really prefer "start" and "end" to "sourceStart" and "sourceEnd"? Usually "start" and "end" refer to indexes in [this]. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:92: list = source.skip(sourceStart).toList(); On 2015/10/14 07:10:00, Lasse Reichstein Nielsen wrote: > This looks wrong - you create a list of only the elements from sourceStart to > sourceEnd, but don't adjust sourceStart/sourceEnd. Done. > 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. Again, this seems contrary to the implementation of ListBase. https://codereview.chromium.org/1404443005/diff/1/lib/typed_buffers.dart#newc... lib/typed_buffers.dart:148: void _setRange(int start, int end, Iterable<E> source, [int skipCount = 0]) { On 2015/10/14 07:10:00, Lasse Reichstein Nielsen wrote: > 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. Done.
> Given that the typed buffers now share API surface that's not just from List, is > it worth making _TypedDataBuffer public? Yes! No! I don't know! It makes sense to make it public - but only if it makes sense to treat a TypedDataBuffer as something general where you could have one, not know which kind it is, and call add on it anyway. We can't extend the List interface itself (sadly). For now, I think we should just not expose it. Florian, what's your opinion? Or should we just make a ListSlice class that is a list and acts like a lazy sublist - so we don't have to have all these annoying [start,end] parameters. (Mmmmm, slice operator! Yum!) https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:75: void addAll(Iterable<E> values, [int start, int end]) { Use [int start = 0, int end] like other range parameters. Basically we treat start as non-nullable and end as nullable. Start out by checking that start >= 0 and end == null || end >= start. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:80: insertAll(_length, values); You could also update insertAll to also take start/end, and just forward to that in all cases. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:84: var list = values is List ? values : values.toList(growable: false); I still don't want to convert the iterable to a list unnecessarily. That's going to run through the iterable anyway, so we might as well do it ourselves and avoid the intermediate allocation. Maybe just: if (values is List) { // smart stuff } else { for (var value in values.getRange(start, end)) { _add(value); } } You can get cleverer than that with the iterable (a tighter loop while there is room in the buffer), but it likely isn't worth it. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); That requires the end to be inside the iterable. We don't generally require that for iterables because the user is not expected to be able to test it efficiently. It should allow a too large start and end, treating it as if you had done iterable.getRange(start, end). The fact that we use a list internally is an optimization, but the semantics should match iterables in general. So: if (values is List) { if (start >= values.length) return; if (end == null || end > values.length) end = values.length; ... code below ... https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:88: _growBuffer(newLength); I prefer _ensureCapacity. The function doesn't actually grow the buffer if the needed capacity is already available, so the name is misleading. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:112: /// the existing data. /// Ensures that [_buffer] is at least [requiredCapacity] long. /// /// Grows the buffer if necessary. void _ensureCapacity(int requiredCapacity) { .... The parameter isn't the required length, it's the required capacity (actual length may be greater). https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... File test/typed_buffers_test.dart (right): https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... test/typed_buffers_test.dart:41: group("with a${type == 'list' ? '' : 'n'} $type", () { You could just make the string "a list" and "an iterable". https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... test/typed_buffers_test.dart:79: expect(() => buffer.addAll(source, 3, -1), throwsRangeError); This is where I think #2 and #4 should not be errors.
Code review changes
https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:75: void addAll(Iterable<E> values, [int start, int end]) { On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > Use [int start = 0, int end] like other range parameters. > Basically we treat start as non-nullable and end as nullable. Done for consistency, although in general default values for non-bools cause more pain than they're worth IME. > Start out by checking that start >= 0 and end == null || end >= start. Done. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:80: insertAll(_length, values); On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > You could also update insertAll to also take start/end, and just forward to that > in all cases. Done. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:84: var list = values is List ? values : values.toList(growable: false); On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > I still don't want to convert the iterable to a list unnecessarily. > That's going to run through the iterable anyway, so we might as well do it > ourselves and avoid the intermediate allocation. > > Maybe just: > if (values is List) { > // smart stuff > } else { > for (var value in values.getRange(start, end)) { > _add(value); > } > } > > You can get cleverer than that with the iterable (a tighter loop while there is > room in the buffer), but it likely isn't worth it. > Done. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > That requires the end to be inside the iterable. We don't generally require that > for iterables because the user is not expected to be able to test it > efficiently. > It should allow a too large start and end, treating it as if you had done > iterable.getRange(start, end). List.getRange doesn't allow too large a start or end. Iterable doesn't have a getRange method, but Iterable.take method does allow too large an index. Also, existing methods in the same vein like List.setRange *do* throw if the iterable doesn't have enough elements—which is basically the same as end being too high. > The fact that we use a list internally is an optimization, but the semantics > should match iterables in general. > > So: > if (values is List) { > if (start >= values.length) return; > if (end == null || end > values.length) end = values.length; > ... code below ... https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:88: _growBuffer(newLength); On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > I prefer _ensureCapacity. The function doesn't actually grow the buffer if the > needed capacity is already available, so the name is misleading. Done. https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:112: /// the existing data. On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > /// Ensures that [_buffer] is at least [requiredCapacity] long. > /// > /// Grows the buffer if necessary. > void _ensureCapacity(int requiredCapacity) { .... Done. > The parameter isn't the required length, it's the required capacity (actual > length may be greater). Okay, I'll also change [_createBiggerBuffer]. https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... File test/typed_buffers_test.dart (right): https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... test/typed_buffers_test.dart:41: group("with a${type == 'list' ? '' : 'n'} $type", () { On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > You could just make the string "a list" and "an iterable". Done. https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... test/typed_buffers_test.dart:79: expect(() => buffer.addAll(source, 3, -1), throwsRangeError); On 2015/10/15 10:58:25, Lasse Reichstein Nielsen wrote: > This is where I think #2 and #4 should not be errors. AFAICT this would be inconsistent with existing methods.
https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); Generally we check the limits on lists and not on iterables (because the user can check the list to avoid invalid lengths, but can't do it for iterables). The List.setRange is inconsistent on that - probably because the range is really on the list and the iterable is just expected to fill it. It's still not optimal. I would actually consider fixing it (either stop early and return the number of elements used, or fill the rest with a default value). https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... File test/typed_buffers_test.dart (right): https://codereview.chromium.org/1404443005/diff/20001/test/typed_buffers_test... test/typed_buffers_test.dart:79: expect(() => buffer.addAll(source, 3, -1), throwsRangeError); Only accidentally. Ranges on iterables should generally be assumed to be equivalent to .take+.skip. https://codereview.chromium.org/1404443005/diff/40001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/1404443005/diff/40001/CHANGELOG.md#newcode3 CHANGELOG.md:3: * Add `start` and `end` parameters to the `addAll()` methods for the typed data Also to `insertAll` https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to [end] (exclusive) in 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]. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:75: void addAll(Iterable<E> values, [int start = 0, int end]) => Don't use => for void functions. Make it a proper { } body without a return. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:82: RangeError.checkNotNegative(start); ...checkNotNegative(start, "start"); https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:84: throw new RangeError.range(start, 0, end, "start"); I'd make this an error of "end": throw new RangeError.range(end, start, null, "end"); https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:90: if (end == null && values is! List) { Just do this if values is not a list. You don't actually know that values has (end-start) elements. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:95: insert(index + i - start, value); This is really horrible - maybe I should do something about it. :) https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:106: throw new StateError("Too few elements"); Don't throw here, the input is an iterable. If start > values.length, just return. If end > values.length, cap it to values.length. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:142: if (requiredCapacity < _buffer.length) return; <= https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:143: var newBuffer = _createBiggerBuffer(null); pass requiredCapacity as parameter?
https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Generally we check the limits on lists and not on iterables (because the user > can check the list to avoid invalid lengths, but can't do it for iterables). > The List.setRange is inconsistent on that - probably because the range is really > on the list and the iterable is just expected to fill it. It's still not > optimal. I would actually consider fixing it (either stop early and return the > number of elements used, or fill the rest with a default value). Wouldn't this be a breaking change? The error is documented behavior. I'd really prefer to match List.setRange's current behavior, even if it's flawed. If it changes later, we can come back and change this as well. But if you insist, I can change it now.
floitsch@google.com changed reviewers: + floitsch@google.com
DBC. (adding even more noise...) https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/20001/lib/typed_buffers.dart#... lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); On 2015/10/20 21:24:39, nweiz wrote: > On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > > Generally we check the limits on lists and not on iterables (because the user > > can check the list to avoid invalid lengths, but can't do it for iterables). > > The List.setRange is inconsistent on that - probably because the range is > really > > on the list and the iterable is just expected to fill it. It's still not > > optimal. I would actually consider fixing it (either stop early and return the > > number of elements used, or fill the rest with a default value). > > Wouldn't this be a breaking change? The error is documented behavior. > > I'd really prefer to match List.setRange's current behavior, even if it's > flawed. If it changes later, we can come back and change this as well. But if > you insist, I can change it now. Since one can leave "end" empty I prefer to check, too. I almost prefer if skip and take were stricter, but then we would need to add a boolean flag (or a different method). https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:90: if (end == null && values is! List) { On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Just do this if values is not a list. > You don't actually know that values has (end-start) elements. I don't understand the comment. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:94: if (i == end) break; This is in the branch where "end" == null. So this can never be true. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:95: insert(index + i - start, value); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > This is really horrible - maybe I should do something about it. :) I'm not sure what you mean, but we can't just insert elements one by one into the middle of a list. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:99: if (i < start) throw new StateError("Too few elements"); 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'). https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:106: throw new StateError("Too few elements"); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Don't throw here, the input is an iterable. > If start > values.length, just return. > If end > values.length, cap it to values.length. 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).
Code review changes
Replying to Florian and some of Lasse's comments I missed before. https://codereview.chromium.org/1404443005/diff/40001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/1404443005/diff/40001/CHANGELOG.md#newcode3 CHANGELOG.md:3: * Add `start` and `end` parameters to the `addAll()` methods for the typed data On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Also to `insertAll` Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to [end] (exclusive) in On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Start with single sentence. 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. > /// Add elements of [values] at the end of this buffer. > > Don't use "This adds", just "Adds". 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. > /// > /// 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]. 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. > 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]. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:75: void addAll(Iterable<E> values, [int start = 0, int end]) => On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > Don't use => for void functions. Make it a proper { } body without a return. Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:82: RangeError.checkNotNegative(start); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > ...checkNotNegative(start, "start"); Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:84: throw new RangeError.range(start, 0, end, "start"); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > I'd make this an error of "end": > throw new RangeError.range(end, start, null, "end"); Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:94: if (i == end) break; On 2015/10/20 22:30:47, floitsch wrote: > This is in the branch where "end" == null. So this can never be true. Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:95: insert(index + i - start, value); On 2015/10/20 22:30:46, floitsch wrote: > On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > > This is really horrible - maybe I should do something about it. :) > > I'm not sure what you mean, but we can't just insert elements one by one into > the middle of a list. 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? https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:99: if (i < start) throw new StateError("Too few elements"); On 2015/10/20 22:30:47, floitsch wrote: > 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'). In this block, we know [end] is null. Below, we rely on the [List.setRange] call to do bounds-checking. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:142: if (requiredCapacity < _buffer.length) return; On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > <= Done. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:143: var newBuffer = _createBiggerBuffer(null); On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > pass requiredCapacity as parameter? Done.
About iterables - we have often sued the guideline that if the user can't test something, then it isn't an error (that's why we have FormatException as an exception). You can't generally test the length of iterables, so I don't think we should err if the length is not what you expected. I don't want to introduce an exception for it, I'd rather just always treat "ranges" of an iterable as equivalent to skip/take. That makes implementation easy - you can just use skip/take internally if you don't have a more efficient thing to do. https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to [end] (exclusive) in On 2015/10/20 22:54:58, nweiz wrote: > On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > > Start with single sentence. > > 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. 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. > > > > Don't start sentence with lower-case letter, even inside brackets: [start]. > > 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. 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.
On 2015/10/21 10:11:18, Lasse Reichstein Nielsen wrote: > About iterables - we have often sued the guideline that if the user can't test > something, then it isn't an error (that's why we have FormatException as an > exception). > > You can't generally test the length of iterables, so I don't think we should err > if the length is not what you expected. I don't want to introduce an exception > for it, > I'd rather just always treat "ranges" of an iterable as equivalent to skip/take. > That makes implementation easy - you can just use skip/take internally if you > don't have a more efficient thing to do. I don't agree. It's fair to do this for skip and take (although it can lead to unexpected errors even there), but these should be the exception. When iterables start to interact with other data structures, then they have to fulfill their role and provide the required number of arguments. > > https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart > File lib/typed_buffers.dart (right): > > https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... > lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to > [end] (exclusive) in > On 2015/10/20 22:54:58, nweiz wrote: > > On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > > > Start with single sentence. > > > > 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. > > 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. > > > > > > > > Don't start sentence with lower-case letter, even inside brackets: [start]. > > > > 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. > > 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.
Code review changes
https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/40001/lib/typed_buffers.dart#... lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to [end] (exclusive) in On 2015/10/21 10:11:18, Lasse Reichstein Nielsen wrote: > On 2015/10/20 22:54:58, nweiz wrote: > > On 2015/10/20 08:09:49, Lasse Reichstein Nielsen wrote: > > > Start with single sentence. > > > > 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. > > 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. Doing an experiment, it looks like I'm wrong. Maybe I'm thinking of outdated docgen behavior? > 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. I've talked to Kathy and she agrees. Done.
Ok, let's make it an error to not have enough elements then. We will still fail at a point where the result is in an inconsistent and unpredictable state. https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:82: insertAll(_length, values, start, end); I generally prefer to not define one public API method in terms of another because it's fragile if someone tries to overload both (e.g., add logging to the call, then you get logging for both). In cases like this, I'd use a private _insertAll called from here and from insertAll. https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough to extract a slice Don't specify the exact errors, just: The [start] value must be non-negative. The [values] iterable must have at least [start] elements, and if [end] is specified, it must be greater than or equal to [start] and [values] must have at least [end] elements. https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:94: void insertAll(int index, Iterable<E> values, [int start = 0, int end]) { Maybe split this earlier if [values] is a list: if (values is List) { end = RangeError.checkValidRange(start, end, values.length); ... do the list thing ... return; } else { _insertAllIterable(index, values, start, end); } void _insertAllIterable(index, values, start, end) { ... do the iterable checks and insertion. With a risk of being inconsistent, doing a toList on the iterable does make sense for insertAll when it's not at the end. At the end, it is still possible to add iteratively without knowing the count, but inserting in the middle has potential non-linear behavior in that case. So, iterating on the design, I still want start/end on both addAll and insertAll, but maybe it's insertAll that should delegate to addAll when the insertion index is at the end. void insertAll(...) { if (index == length) { _addAll(valus, start, end); return; } int originalStart = start; int originalEnd = end; if (values is! List) { if (end != null) { values = values.take(end); end -= start; } values = values.skip(start).toList(); start = 0; } end = RangeError.checkValidRange(origialStart, originalStart, list.length + originalStart) - originalStart; ... efficient list-based insert and range checks ... } void addAll(Iterable values, [start = 0, end]) { _addAll(values, start, end); } void _addAll(...) { if (values is List) { efficient list based add. } else { RangeError.checkNotNegative(start, "start"); if (end != null) { RangeError.checkValidRange(end, start, null, "end"); values = values.take(end); } int i = 0; for (var value in values) { if (i >= start) _add(value); i++; } if (i < start) { throw new RangeError.range(start, 0, i, "start"); } if (end != null && i < end) { throw new RangeError.range(end, start, i, "end"); } } } https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:110: if (i < start) throw new StateError("Too few elements"); It's the [values] argument that have too few arguments, not "this", so I'd make it an argument error not a state error. For consistency, I'd do: new RangeError.range(start, 0, i, "start"); https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:117: throw new StateError("Too few elements"); RangeError.range(end, start, values.length, "end");
Code review changes
https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:82: insertAll(_length, values, start, end); On 2015/10/23 08:00:02, Lasse Reichstein Nielsen wrote: > I generally prefer to not define one public API method in terms of another > because it's fragile if someone tries to overload both (e.g., add logging to the > call, then you get logging for both). > In cases like this, I'd use a private _insertAll called from here and from > insertAll. Done. https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough to extract a slice On 2015/10/23 08:00:02, Lasse Reichstein Nielsen wrote: > Don't specify the exact errors, just: > > The [start] value must be non-negative. > The [values] iterable must have at least [start] elements, and if > [end] is specified, it must be greater than or equal to [start] and > [values] must have at least [end] elements. Done, but why? Isn't it good for the user to know what errors to expect? https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:94: void insertAll(int index, Iterable<E> values, [int start = 0, int end]) { On 2015/10/23 08:00:02, Lasse Reichstein Nielsen wrote: > Maybe split this earlier if [values] is a list: > > if (values is List) { > end = RangeError.checkValidRange(start, end, values.length); > ... do the list thing ... > return; > } else { > _insertAllIterable(index, values, start, end); > } > > void _insertAllIterable(index, values, start, end) { > ... do the iterable checks and insertion. > > > With a risk of being inconsistent, doing a toList on the iterable does make > sense for insertAll when it's not at the end. At the end, it is still possible > to add iteratively without knowing the count, but inserting in the middle has > potential non-linear behavior in that case. > > So, iterating on the design, I still want start/end on both addAll and > insertAll, but maybe it's insertAll that should delegate to addAll when the > insertion index is at the end. > > void insertAll(...) { > if (index == length) { > _addAll(valus, start, end); > return; > } > int originalStart = start; > int originalEnd = end; > if (values is! List) { > if (end != null) { > values = values.take(end); > end -= start; > } > values = values.skip(start).toList(); > start = 0; > } > end = RangeError.checkValidRange(origialStart, originalStart, > list.length + originalStart) - originalStart; > ... efficient list-based insert and range checks ... > } > void addAll(Iterable values, [start = 0, end]) { > _addAll(values, start, end); > } > void _addAll(...) { > if (values is List) { > efficient list based add. > } else { > RangeError.checkNotNegative(start, "start"); > if (end != null) { > RangeError.checkValidRange(end, start, null, "end"); > values = values.take(end); > } > int i = 0; > for (var value in values) { > if (i >= start) _add(value); > i++; > } > if (i < start) { > throw new RangeError.range(start, 0, i, "start"); > } > if (end != null && i < end) { > throw new RangeError.range(end, start, i, "end"); > } > } > } Done. https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:110: if (i < start) throw new StateError("Too few elements"); On 2015/10/23 08:00:02, Lasse Reichstein Nielsen wrote: > It's the [values] argument that have too few arguments, not "this", so I'd make > it an argument error not a state error. > For consistency, I'd do: > new RangeError.range(start, 0, i, "start"); This is inconsistent with the corresponding error raised by setRange: https://github.com/dart-lang/sdk/blob/master/sdk/lib/collection/list.dart#L380
LGTM! https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:96: RangeError.checkValidIndex(index, this, "index", _length + 1); Tricky, this "index" is really not an index (0<=index<length). I guess it's ok to use checkValidIndex anyway since it's named "index" and the error message will be correct. https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:105: // center because copying the trailing elements every time is non-linear. There are things we can do which is linear, but it will require some copying (basically this.end-index + min(this.end-index, end-start) elements copies, and it requires a growing buffer on the side). https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:114: values = values.toList(growable: false); do: if (end == null) { if (values is! List) { values = values.skip(start).toList(); start = 0; } end = values.length; } to avoid wasting space for the values before start and ensure end is set before calling _insertKnownLength. https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:126: RangeError.checkNotNegative(start, "start"); We already checked this for insertAll. Move these checks to addAll, before calling _addAll, and remove them here. https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:133: // other well-known types. How about: if (values is List) { end ??= values.length; } if (end != null) { _insertKnownLength(...); } Then you can avoid the redundant (values is List) check in _insertKnownLength because it's always called with a non-null end.
https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough to extract a slice On 2015/10/26 23:53:57, nweiz wrote: > On 2015/10/23 08:00:02, Lasse Reichstein Nielsen wrote: > > Don't specify the exact errors, just: > > > > The [start] value must be non-negative. > > The [values] iterable must have at least [start] elements, and if > > [end] is specified, it must be greater than or equal to [start] and > > [values] must have at least [end] elements. > > Done, but why? Isn't it good for the user to know what errors to expect? It really matters for exceptions, but not for errors. The error-class should only matters for representation. That is, how the toString works, and what it captures to give better debugging information. The program itself should never care. By not specifying which error, it allows us to switch to a better fitting error in the future (if we introduce one).
https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough to extract a slice What Florian said! https://codereview.chromium.org/1404443005/diff/80001/lib/typed_buffers.dart#... lib/typed_buffers.dart:110: if (i < start) throw new StateError("Too few elements"); So it is. I'm getting less and less enchanted by the setRange implementation.
Code review changes
https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:114: values = values.toList(growable: false); On 2015/10/27 10:10:19, Lasse Reichstein Nielsen wrote: > do: > > if (end == null) { > if (values is! List) { > values = values.skip(start).toList(); > start = 0; > } > end = values.length; > } > > to avoid wasting space for the values before start and ensure end is set before > calling _insertKnownLength. Won't calling [skip] avoid throwing an error when start = end > values.length? https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:126: RangeError.checkNotNegative(start, "start"); On 2015/10/27 10:10:18, Lasse Reichstein Nielsen wrote: > We already checked this for insertAll. Move these checks to addAll, before > calling _addAll, and remove them here. Done. https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:133: // other well-known types. On 2015/10/27 10:10:18, Lasse Reichstein Nielsen wrote: > How about: > > if (values is List) { > end ??= values.length; > } > if (end != null) { > _insertKnownLength(...); > } > > Then you can avoid the redundant (values is List) check in _insertKnownLength > because it's always called with a non-null end. Done.
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 4a1eb49d4f2576044d71f2dd6c3f8e619f6678a9 (presubmit successful).
Message was sent while issue was closed.
On 2015/10/27 21:27:54, nweiz wrote: > Committed patchset #7 (id:120001) manually as > 4a1eb49d4f2576044d71f2dd6c3f8e619f6678a9 (presubmit successful). And publish?
Message was sent while issue was closed.
https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart File lib/typed_buffers.dart (right): https://codereview.chromium.org/1404443005/diff/100001/lib/typed_buffers.dart... lib/typed_buffers.dart:114: values = values.toList(growable: false); True. I might try to optimize this more - it annoys me that it is storing the intermediate numbers in a non-typed-data List, but I guess it's fine for now. |