Add typed queue classes #21

nex3 · 2019-10-18T09:10:50Z

These make it easy to, for example, efficiently enqueue incoming
chunked binary data for processing.

These make it easy to, for example, efficiently enqueue incoming chunked binary data for processing.

nshahan · 2019-10-18T16:50:03Z

I'm not familiar with this package.

@eernstg I think you are in the process of migrating dart:typed_data. Would you also want to review this addition to package:typed_data with your API changes in mind?

eernstg · 2019-10-18T18:40:19Z

Hi @nshahan and @nex3,
I think @lrhn would be the obvious choice for such a review—he has been the maintainer of the core libraries for a long time.

@lrhn, wdyt? We could split the work, too.

nex3 · 2019-11-06T21:52:04Z

Ping!

lrhn

Looks very good!

lrhn · 2019-11-07T08:43:36Z

lib/src/typed_queue.dart

+  int get length => (_tail - _head) & (_table.length - 1);
+
+  List<E> toList({bool growable = true}) {
+    var list = growable ? (<E>[]..length = length) : _createList(length);


Might as well start to prepare for NNBD. The ..length = length won't work there, you can't grow a non-nullable list.
(Usual workaround is List<E>.filled(length, ..someDefaultValue..), where the default value here is probably either 0 or 0.0/NaN depending on E).
Maybe have a E get _defaultValue; which you can override to return the value to use.

Since we are in package:typed_data, we have access to growable typed data, so consider returning a Uint8Queue or Uint8Buiffer instead of a plain growable list. Both are valid lists.

lib/src/typed_queue.dart

lrhn · 2019-11-07T08:53:43Z

lib/src/typed_queue.dart

+    var delta = value - length;
+    if (delta >= 0) {
+      if (_table.length <= value) _growTo(value);
+      _tail = (_tail + delta) & (_table.length - 1);


You should also fill the range with zeros (at least when you don't grow).

Since removeLast doesn't clear the entry it removed, the data is still in the backing store. Increasing the length will make it accessible again, which seems like something that users could end up depending on, even if it's not actually promised. Better to zero out the range before making it available.

lrhn · 2019-11-07T08:56:49Z

lib/src/typed_queue.dart

+  }
+
+  E operator [](int index) {
+    RangeError.checkValidIndex(index, this);


Use RangeError.checkValidIndex(index, this, null, this.length);.
If you don't pass the length, it is found through a dynamic lookup on this. That's probably not good for performance any more.

Done. That's probably a good note to add to the docs.

lrhn · 2019-11-07T09:01:21Z

lib/src/typed_queue.dart

+
+  L sublist(int start, [int end]) {
+    var length = this.length;
+    RangeError.checkValidRange(start, end, length);


end = RangeError.checkValidRange(start, end, length);

will assign end to length or itself.

lrhn · 2019-11-07T09:08:09Z

lib/src/typed_queue.dart

+
+  // Create a new typed list.
+  L _createList(int size);
+}


This entire class seems to be indifferent to it being intended for typed data.

Consider making it publicly available as a general ListQueue skeleton class, e.g., which takes the implementation of _createList as a function argument to the constructor.

I think that could be a good addition to package:collection (or dart:collection, which implements QueueList basically the same way), but unfortunately I don't have the bandwidth for it.

lrhn · 2019-11-07T09:08:57Z

lib/src/typed_queue.dart

+import "package:collection/collection.dart";
+
+/// The shared superclass of all the typed queue subclasses.
+abstract class _TypedQueue<E, L extends List<E>> extends Object


extends Object with ListMixin<E> can now be written as just with ListMixin<E>.

lrhn · 2019-11-07T09:09:50Z

lib/src/typed_queue.dart

+  /// The underlying data buffer.
+  ///
+  /// This is always both a List<E> and a TypedData, which we don't have a type
+  /// for that. For example, for a `Uint8Buffer`, this is a `Uint8List`.


Uint8Buffer -> Uint8Queue

?

lrhn · 2019-11-07T09:12:32Z

lib/src/typed_queue.dart

+  L _createList(int size);
+}
+
+abstract class _IntQueue<L extends List<int>> extends _TypedQueue<int, L> {


Is this class necessary?
It doesn't seem to be used for anything except writing
extends _IntQueue<Uint8List>
instead of
extends _TypedQueue<int, Uint8List>
or similar for each actual class.

(If you had a _defaultValue getter, it would make sense to have it return 0 here).

We have _defaultValue now.

test/queue_test.dart

lrhn

LGTM. Thanks!

lib/src/typed_queue.dart

kevmoo · 2020-01-12T01:40:17Z

What's the story here?

nex3 · 2020-01-13T23:59:10Z

Sorry, this dropped off my radar. I'll get it mergeable some time this week.

nex3 · 2020-01-15T19:46:48Z

A bunch of new lints were added since I opened this. I've fixed a few one-offs, but the remainders have many failures and no way to do automated fixes. Does someone else care enough to fix them, or can I just comment them out for now?

kevmoo · 2020-01-15T20:30:39Z

A bunch of new lints were added since I opened this. I've fixed a few one-offs, but the remainders have many failures and no way to do automated fixes. Does someone else care enough to fix them, or can I just comment them out for now?

Just pushed the fixes for you.

dart bin/dartfix.dart --fix=prefer-single-quotes --fix=annotate-overrides --required --pedantic -w ~/github/typed_data/

From /sdk/pkg/dartfix

lrhn · 2020-01-16T06:41:45Z

pubspec.yaml

@@ -9,6 +9,9 @@ homepage: https://github.com/dart-lang/typed_data
 environment:
  sdk: '>=2.4.0 <3.0.0'

+dependencies:
+  collection: ^1.1.0
+
 dev_dependencies:
  pedantic: ^1.0.0


Could also just not depend on pedantic, or lock it to pedantic: 1.7.0 are thereabout.
The newest versions of pedantic have jumped the shark, and I do not want to support the lints they have chose to enable.

lrhn · 2020-01-16T06:43:45Z

lib/src/typed_queue.dart

@@ -27,17 +27,20 @@ abstract class _TypedQueue<E, L extends List<E>> with ListMixin<E> {

  // Iterable interface.

+  @override


Please don't add @OverRide.
Please don't change quotes.

(So, in general, no to all these changes).

This reverts commit 5b3b1f3.

Add typed queue classes

Add typed queue classes

f9d5dad

These make it easy to, for example, efficiently enqueue incoming chunked binary data for processing.

nex3 requested a review from nshahan October 18, 2019 09:10

googlebot added the cla: yes label Oct 18, 2019

nex3 added 2 commits October 18, 2019 10:23

Ignore an unhelpful lint

9d7701e

Run tests on stable and dev

e4d231c

lrhn approved these changes Nov 7, 2019

View reviewed changes

Code review changes

ef67657

lrhn approved these changes Nov 15, 2019

View reviewed changes

lib/src/typed_queue.dart Show resolved Hide resolved

More fixes

3c54f8c

nex3 added 2 commits January 14, 2020 16:23

Merge remote-tracking branch 'origin/master' into queue

296526d

Fix a couple lints

1035351

dartfix

5b3b1f3

lrhn reviewed Jan 16, 2020

View reviewed changes

nex3 added 2 commits January 16, 2020 13:03

Revert "dartfix"

4307526

This reverts commit 5b3b1f3.

Avoid newer pedantic lints, per request from @lrhn

c82dcdc

lrhn merged commit 0ba3ca8 into master Jan 17, 2020

kevmoo deleted the queue branch February 27, 2020 00:04

mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 16, 2024

Merge pull request dart-archive/typed_data#21 from dart-lang/queue

0d0d11e

Add typed queue classes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typed queue classes #21

Add typed queue classes #21

nex3 commented Oct 18, 2019

nshahan commented Oct 18, 2019

eernstg commented Oct 18, 2019

nex3 commented Nov 6, 2019

lrhn left a comment

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019 •

edited

Loading

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn Nov 7, 2019

nex3 Nov 14, 2019

lrhn left a comment

kevmoo commented Jan 12, 2020

nex3 commented Jan 13, 2020

nex3 commented Jan 15, 2020

kevmoo commented Jan 15, 2020

lrhn Jan 16, 2020

nex3 Jan 16, 2020

lrhn Jan 16, 2020

nex3 Jan 16, 2020

		@@ -27,17 +27,20 @@ abstract class _TypedQueue<E, L extends List<E>> with ListMixin<E> {

		// Iterable interface.

		@override

Add typed queue classes #21

Add typed queue classes #21

Conversation

nex3 commented Oct 18, 2019

nshahan commented Oct 18, 2019

eernstg commented Oct 18, 2019

nex3 commented Nov 6, 2019

lrhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn left a comment

Choose a reason for hiding this comment

kevmoo commented Jan 12, 2020

nex3 commented Jan 13, 2020

nex3 commented Jan 15, 2020

kevmoo commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn Nov 7, 2019 •

edited

Loading