Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(95)

Issue 1404443005: Add _TypedDataBuffer.addRange. (Closed)

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.

Description

Add _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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+231 lines, -12 lines) Patch
M CHANGELOG.md View 1 2 3 1 chunk +9 lines, -2 lines 0 comments Download
M lib/typed_buffers.dart View 1 2 3 4 5 6 3 chunks +119 lines, -9 lines 0 comments Download
M pubspec.yaml View 1 chunk +1 line, -1 line 0 comments Download
M test/typed_buffers_test.dart View 1 2 1 chunk +102 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (1 generated)
nweiz
5 years, 2 months ago (2015-10-14 00:04:34 UTC) #1
Lasse Reichstein Nielsen
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#newcode73 lib/typed_buffers.dart:73: void addAll(Iterable<E> values) { I'd rather have specialized list ...
5 years, 2 months ago (2015-10-14 07:10:00 UTC) #2
nweiz
Code review changes
5 years, 2 months ago (2015-10-14 20:19:16 UTC) #3
nweiz
I added some tests, which I guess I forgot last time. Given that the typed ...
5 years, 2 months ago (2015-10-14 20:20:20 UTC) #4
Lasse Reichstein Nielsen
> Given that the typed buffers now share API surface that's not just from List, ...
5 years, 2 months ago (2015-10-15 10:58:25 UTC) #5
nweiz
Code review changes
5 years, 2 months ago (2015-10-15 20:14:03 UTC) #6
nweiz
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#newcode75 lib/typed_buffers.dart:75: void addAll(Iterable<E> values, [int start, int end]) { On ...
5 years, 2 months ago (2015-10-15 20:14:10 UTC) #7
Lasse Reichstein Nielsen
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#newcode85 lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); Generally we check the ...
5 years, 2 months ago (2015-10-20 08:09:50 UTC) #8
nweiz
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#newcode85 lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, list.length); On 2015/10/20 08:09:49, Lasse ...
5 years, 2 months ago (2015-10-20 21:24:39 UTC) #9
floitsch
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#newcode85 lib/typed_buffers.dart:85: end = RangeError.checkValidRange(start, end, ...
5 years, 2 months ago (2015-10-20 22:30:47 UTC) #11
nweiz
Code review changes
5 years, 2 months ago (2015-10-20 22:54:41 UTC) #12
nweiz
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): ...
5 years, 2 months ago (2015-10-20 22:54:58 UTC) #13
Lasse Reichstein Nielsen
About iterables - we have often sued the guideline that if the user can't test ...
5 years, 2 months ago (2015-10-21 10:11:18 UTC) #14
floitsch
On 2015/10/21 10:11:18, Lasse Reichstein Nielsen wrote: > About iterables - we have often sued ...
5 years, 2 months ago (2015-10-21 10:16:47 UTC) #15
nweiz
Code review changes
5 years, 2 months ago (2015-10-21 21:22:26 UTC) #16
nweiz
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#newcode73 lib/typed_buffers.dart:73: /// This adds values from [start] (inclusive) to [end] ...
5 years, 2 months ago (2015-10-21 21:22:30 UTC) #17
Lasse Reichstein Nielsen
Ok, let's make it an error to not have enough elements then. We will still ...
5 years, 1 month ago (2015-10-23 08:00:02 UTC) #18
nweiz
Code review changes
5 years, 1 month ago (2015-10-26 23:53:51 UTC) #19
nweiz
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#newcode82 lib/typed_buffers.dart:82: insertAll(_length, values, start, end); On 2015/10/23 08:00:02, Lasse Reichstein ...
5 years, 1 month ago (2015-10-26 23:53:57 UTC) #20
Lasse Reichstein Nielsen
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#newcode96 lib/typed_buffers.dart:96: RangeError.checkValidIndex(index, this, "index", _length + 1); Tricky, this ...
5 years, 1 month ago (2015-10-27 10:10:19 UTC) #21
floitsch
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#newcode92 lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough ...
5 years, 1 month ago (2015-10-27 10:26:17 UTC) #22
Lasse Reichstein Nielsen
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#newcode92 lib/typed_buffers.dart:92: /// Throws a [StateError] if [values] isn't long enough ...
5 years, 1 month ago (2015-10-27 10:33:12 UTC) #23
nweiz
Code review changes
5 years, 1 month ago (2015-10-27 21:25:50 UTC) #24
nweiz
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#newcode114 lib/typed_buffers.dart:114: values = values.toList(growable: false); On 2015/10/27 10:10:19, Lasse Reichstein ...
5 years, 1 month ago (2015-10-27 21:26:08 UTC) #25
nweiz
Committed patchset #7 (id:120001) manually as 4a1eb49d4f2576044d71f2dd6c3f8e619f6678a9 (presubmit successful).
5 years, 1 month ago (2015-10-27 21:27:54 UTC) #26
kevmoo
On 2015/10/27 21:27:54, nweiz wrote: > Committed patchset #7 (id:120001) manually as > 4a1eb49d4f2576044d71f2dd6c3f8e619f6678a9 (presubmit ...
5 years, 1 month ago (2015-10-28 02:58:34 UTC) #27
Lasse Reichstein Nielsen
5 years, 1 month ago (2015-10-28 07:31:57 UTC) #28
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.

Powered by Google App Engine
This is Rietveld 408576698