Fix issues surrounding type checking for Uint8Arrays and Buffers #346
Conversation
| @@ -312,6 +312,7 @@ function fromArrayBuffer (array, byteOffset, length) { | |||
|
|
|||
| function fromObject (obj) { | |||
| if (Buffer.isBuffer(obj)) { | |||
| // Note: Probably not necessary anymore. | |||
There was a problem hiding this comment.
What isn't necessary about this now?
(citation?)
There was a problem hiding this comment.
Because ArrayBuffer.isView returns true for Uint8Array subclasses like Buffer. So this case will actually be handled earlier, in Buffer.from:
if (ArrayBuffer.isView(value)) {
return fromArrayView(value)
}I left the Buffer.isBuffer(obj) check in there because I was concerned that maybe non-v8 javascript engines' ArrayBuffer.isView does not return true for Uint8Array subclasses (from the same context or another).
Because ArrayByffer.isView is so magic in v8, I actually wanted to test it in FF and Safari to make sure they had the same behavior, but I didn't get around to it, so leaving the check in is just me being cautious.
There was a problem hiding this comment.
You should add comments like ^ into your code 💙
There was a problem hiding this comment.
Yeah, it's something I need to work on.
|
@hedi-edelbloute, it shouldn't affect |
|
@chjj I'm going to provide infos on the issue then, thanks |
Logic behind this:
b instanceof Uint8Arrayalways returns true for Buffers created in the same context.ArrayBuffer.isView(b)returns true for both Uint8Arrays and Buffers created in any context (at least in v8 -- hopefully other browsers too).isInstance(b, Uint8Array)returns true for Uint8rrays created in any context but currently only returns true for Buffers created in the same context.Number 3 has an inconsistency. This PR seeks to rectify that and also optimize several functions in doing so.
If we modify the code such that
isInstance(b, Uint8Array)returns true for Buffers created in all contexts, we can drop a ton of the "buffer casting" for Uint8Arrays (which in most cases is actually entirely redundant and causes a slowdown -- for example, inBuffer.compare). I suspect a lot of this code is actually left over from the days when buffer.js still supported IE6.Not only does this fix a few bugs (e.g.
Buffer#equalsandBuffer#copynot accepting Uint8Arrays as their target), but it can also optimize a few functions by avoiding the buffer casts.In effect, we treat Buffers and Uint8Arrays (from any context) as though they were of the exact same class, which is what node.js does.