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

Fix issues surrounding type checking for Uint8Arrays and Buffers #346

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

chjj
Copy link
Contributor

@chjj chjj commented Feb 3, 2024

Logic behind this:

  1. b instanceof Uint8Array always returns true for Buffers created in the same context.
  2. ArrayBuffer.isView(b) returns true for both Uint8Arrays and Buffers created in any context (at least in v8 -- hopefully other browsers too).
  3. 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, in Buffer.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#equals and Buffer#copy not 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.

index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
Copy link
Collaborator

@dcousens dcousens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -312,6 +312,7 @@ function fromArrayBuffer (array, byteOffset, length) {

function fromObject (obj) {
if (Buffer.isBuffer(obj)) {
// Note: Probably not necessary anymore.
Copy link
Collaborator

@dcousens dcousens Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What isn't necessary about this now?
(citation?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add comments like ^ into your code 💙

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's something I need to work on.

@dcousens dcousens merged commit 5ac5ac4 into feross:master Feb 8, 2024
6 checks passed
@hedi-edelbloute
Copy link

Hi @chjj, your update looks well ! Thanks

Do you know if it could fix this issue #329 ? 🤔

@chjj
Copy link
Contributor Author

chjj commented Mar 1, 2024

@hedi-edelbloute, it shouldn't affect subarray at all. feross/buffer doesn't ever try to patch or change subarray, but if you are experiencing the issue described in #329, please post the affected javascript engine and version in that issue.

@hedi-edelbloute
Copy link

@chjj I'm going to provide infos on the issue then, thanks

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

Successfully merging this pull request may close these issues.

3 participants