Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ByteBuffer an active object with a useful interface, not an opaque capability. #16130

Closed
lrhn opened this issue Jan 16, 2014 · 4 comments
Closed
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-typed-data type-enhancement A request for a change that isn't a bug

Comments

@lrhn
Copy link
Member

lrhn commented Jan 16, 2014

Currently the ByteBuffer object has no useful methods, and the interface cannot be implemented by new classes because the system depends on an internal shared secret between ByteBuffer and the .view constructors.

How about instead having:
  class ByteBuffer {
    ...
    Int8List asInt8List([int start = 0, int end]);
    Uint8List asUint8List([int start = 0, int end]);
    ...
    Float64List asFloat64List([int start = 0, int end]);
    ByteData asByteData([int start = 0, int end]);
  }

and make the view constructors go that way instead:
  factory Uint8List.view(ByteBuffer buffer, [int start = 0, int end]) =>
      buffer.asUint8List(start, end);

That would mean that anyone can create a new ByteBuffer implementation (or mock), and that there doesn't need to be an assumption in the view constructors on the actual implementation of the buffer.
(The view constructors are irrelevant now, but we can keep them for backwards compatibility).
  

@rakudrama
Copy link
Member

I like this idea in general.
It might be more discoverable in the editor too (via code completion)

We must be careful: ByteBuffer must correspond to ArrayBuffer in the Typed Array Specification. http://www.khronos.org/registry/typedarray/specs/latest/#­5
I don't think this proposal has any problems in this regard.
(The main thing to avoid is adding any data access methods to ByteBuffer.)

As proposed, the signatures are slightly wrong: the last parameter is a length measured in the view unit, defaulting to the maximum legal view; for example, you get a view of 2 Uint32 values out of a 10 byte buffer starting at offset 0.

Maybe the methods should still contain the word 'view'

  abstract class ByteBuffer {
    ...
    Int8List viewAsInt8List([int offsetInBytes = 0, int length]);
    Uint8List viewAsUint8List([int offsetInBytes = 0, int length]);
    ...
    Float64List viewAsFloat64List([int offsetInBytes = 0, int length]);
    ByteData viewAsByteData([int offsetInBytes = 0, int length]);
  }

  factory Uint8List.view(ByteBuffer buffer, [int offsetInBytes = 0, int length]) =>
      buffer.viewAsUint8List(offsetInBytes, length);

If we make this change, I would want to mark the T.view constructors as deprecated, and find some way to document for each T that a T can be created from a ByteBuffer.

One thing that is slightly more tricky: dart2js could easily see an explicit length argument to the constructor and do better type analysis and eliminate bounds checks from fixed-sized views. This is more difficult with the indirection.

@rakudrama
Copy link
Member

It appears that this suggestion is implemented in https://code.google.com/p/dart/source/detail?r=37974

  1. Should we mark the matching factory constructors as deprecated?
  2. I think the documentation on the methods could be improved

  /**
   * Creates a new [Uint32List] view of this buffer.
   */
  Uint32List asUint32List([int offsetInBytes = 0, int length]);

This contains much less information than the matching factory constructor

  /**
   * Creates a [Uint32List] view of the specified region in
   * the specified byte buffer. Changes in the [Uint32] will be
   * visible in the byte buffer and vice versa. If the [offsetInBytes] index
   * of the region is not specified, it defaults to zero (the first byte in
   * the byte buffer). If the length is not specified, it defaults to null,
   * which indicates that the view extends to the end of the byte buffer.
   *
   * Throws [RangeError] if [offsetInBytes] or [length] are negative, or
   * if [offsetInBytes] + ([length] * elementSizeInBytes) is greater than
   * the length of [buffer].
   *
   * Throws [ArgumentError] if [offsetInBytes] is not a multiple of
   * [BYTES_PER_ELEMENT].
   */
  factory Uint32List.view(ByteBuffer buffer,
                          [int offsetInBytes = 0, int length]) {
    return buffer.asUint32List(offsetInBytes, length);
  }

The asXXX method should contain the same information, perhaps simplifying the factory constructors to say they are the same as the asXXX methods.

I would also suggest improving the original descriptions with an additional paraphrasing of the constraints, i.e. the view is within the bounds of the buffer, the view is must be aligned to the element type.

@rakudrama
Copy link
Member

cc @lrhn.

@lrhn lrhn added Type-Enhancement library-typed-data area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Mar 2, 2015
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed priority-unassigned labels Feb 29, 2016
@lrhn
Copy link
Member Author

lrhn commented Aug 11, 2017

That's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-typed-data type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants