Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Uint8Array(uint8arrayInstance) does not copy. #4714

Closed
creationix opened this issue Feb 4, 2013 · 18 comments
Closed

Uint8Array(uint8arrayInstance) does not copy. #4714

creationix opened this issue Feb 4, 2013 · 18 comments
Assignees

Comments

@creationix
Copy link

I ran into a bug in cifre when running node, where creating a new Uint8Array from another caused the new instance to share the same backing store as the original. In the browser this does not happen unless you explicitly pass in the underlying ArrayBuffer of the original into the second's constructor.

var a = new Uint8Array([1,2,3,4]);
var b = new Uint8Aray(a);
b[0] = 5;

// Expected a[0] --> 1
// Actual   a[0] --> 5
@creationix
Copy link
Author

Here is the workaround I've done for now in cifre: hookflash/obsolete.cifre@398ff3f

@trevnorris
Copy link

From the spec:

If the input array is a TypedArray, the two arrays may use the same underlying array buffer.

So it's up to the implementation to decide how this is done.

@bnoordhuis
Copy link
Member

@trevnorris is technically correct - the best kind of correct - but it's probably best to follow browser behavior here. Principle of least surprise and all that.

@bnoordhuis
Copy link
Member

Can someone review bnoordhuis/node@94d2ad0?

@bnoordhuis
Copy link
Member

Fixed, for better or worse, in 01ee551.

@creationix
Copy link
Author

FWIW, I think the browser behavior that node will soon match is better. In practice there are many times I want to share the backing store and many times I want a copy. Being able to choose by either using a Uint8Array or it's ArrayBuffer makes working with these things easier.

@trevnorris
Copy link

@bnoordhuis forgot to implement the same with DataViews:

var ui = new Uint8Array(8);
var dv = new DataView(ui);

ui[0] === 0

dv.setUint8(0, 8);

ui[0] === 8;

@Mithgol
Copy link

Mithgol commented Feb 6, 2013

DataViews are meant to be interfaces, not copies.

@trevnorris
Copy link

@Mithgol I realize that, but take into account:

The DataView view provides a low-level interface for reading such data from and writing it to an ArrayBuffer.

So technically DataView's shouldn't be able to read in a TypedArray. Only an ArrayBuffer. There's a footnote that states:

Should DataView have methods for reading arrays of data?
While such methods would be useful, they are not present in this version of the specification to reduce the API footprint. Such methods may be added in the future.

But since node allows for it, think it's in the best interest that DataViews follow the TypedArray specification.

@bnoordhuis
Copy link
Member

Okay, fixed in 7b0770b. I guess it's technically a violation of the spec but we're not supposed to accept anything besides ArrayBuffers in the DataView constructor anyway.

I'm reopening the issue because there's still a bug that needs to be addressed: the .buffer property on the array/view object points to the original, not the copy.

@bnoordhuis
Copy link
Member

Original fixes reverted in fd9d8b5 and 144e21e, proper fixes landed in 234551a and fe10335. Note that the DataView constructor now follows the spec (and the browsers) and no longer accepts typed arrays or buffers.

@trevnorris
Copy link

@bnoordhuis Allowing typed arrays and data views to accept buffers is a nicety that was added pre-v0.8. See GH-4257. Think that specific change will need to be reverted.

@Mithgol
Copy link

Mithgol commented Feb 10, 2013

the DataView constructor now follows the spec (and the browsers) and no longer accepts typed arrays or buffers

For the future reference, that's how you have just broken my jDataView/jDataView#26 simplification of the DataView module.

@bnoordhuis
Copy link
Member

Typed arrays still accept buffers.

The reason the DataView constructor no longer does is that it creates some subtle semantic differences: view = new DataView(array_buffer) creates a view where view.buffer === array_buffer. You can't stick a Buffer in view.buffer because it's not an ArrayBuffer and copying the buffer is not an option either because then the DataView is no longer a view.

Ergo, allowing buffers was a mistake and one that should be rectified sooner rather than later.

@bnoordhuis
Copy link
Member

And before someone brings it up, yes, you could make a Buffer object look like an ArrayBuffer by giving it a byteLength property but that doesn't make it one. For starters, the slice() method works different.

@trevnorris
Copy link

@bnoordhuis please update Buffer documentation that states Buffers can be used with DataViews.

@bnoordhuis
Copy link
Member

Yes, yes. Done in 30b0bc4.

@trevnorris
Copy link

for posterity, this discussion is continued in GH-4742.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants