-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Fixes an issue where Buffer.from was not supporting SharedArrayBuffer #8510
Changes from all commits
699b605
7a1e765
ff3555f
ce9aa0b
a66a3e0
bdfaa52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
'use strict'; | ||
|
||
const binding = process.binding('buffer'); | ||
const { isArrayBuffer } = process.binding('util'); | ||
const { isArrayBuffer, isSharedArrayBuffer } = process.binding('util'); | ||
const bindingObj = {}; | ||
const internalUtil = require('internal/util'); | ||
|
||
|
@@ -103,7 +103,7 @@ Buffer.from = function(value, encodingOrOffset, length) { | |
if (typeof value === 'number') | ||
throw new TypeError('"value" argument must not be a number'); | ||
|
||
if (isArrayBuffer(value)) | ||
if (isArrayBuffer(value) || isSharedArrayBuffer(value)) | ||
return fromArrayBuffer(value, encodingOrOffset, length); | ||
|
||
if (typeof value === 'string') | ||
|
@@ -264,7 +264,8 @@ function fromObject(obj) { | |
} | ||
|
||
if (obj) { | ||
if (isArrayBuffer(obj.buffer) || 'length' in obj) { | ||
if (isArrayBuffer(obj.buffer) || 'length' in obj || | ||
isSharedArrayBuffer(obj)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ojss Should this have been There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addaleax you're right! I am so sorry about this mistake, I will fix this right away. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, sorry I didn’t notice that during review myself! :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while we're at it can we do something about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wait. this already landed. okay, ill take care of it another time. |
||
if (typeof obj.length !== 'number' || obj.length !== obj.length) { | ||
return new FastBuffer(); | ||
} | ||
|
@@ -351,8 +352,10 @@ function base64ByteLength(str, bytes) { | |
|
||
function byteLength(string, encoding) { | ||
if (typeof string !== 'string') { | ||
if (ArrayBuffer.isView(string) || isArrayBuffer(string)) | ||
if (ArrayBuffer.isView(string) || isArrayBuffer(string) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed the style and added |
||
isSharedArrayBuffer(string)) { | ||
return string.byteLength; | ||
} | ||
|
||
string = '' + string; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/*global SharedArrayBuffer*/ | ||
'use strict'; | ||
// Flags: --harmony-sharedarraybuffer | ||
|
||
require('../common'); | ||
const assert = require('assert'); | ||
const Buffer = require('buffer').Buffer; | ||
|
||
const sab = new SharedArrayBuffer(24); | ||
const arr1 = new Uint16Array(sab); | ||
const arr2 = new Uint16Array(12); | ||
arr2[0] = 5000; | ||
arr1[0] = 5000; | ||
arr1[1] = 4000; | ||
arr2[1] = 4000; | ||
|
||
const arr_buf = Buffer.from(arr1.buffer); | ||
const ar_buf = Buffer.from(arr2.buffer); | ||
|
||
assert.deepStrictEqual(arr_buf, ar_buf, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can probably leave the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Being explicit is better when checking for things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
arr1[1] = 6000; | ||
arr2[1] = 6000; | ||
|
||
assert.deepStrictEqual(arr_buf, ar_buf, 0); | ||
|
||
// Checks for calling Buffer.byteLength on a SharedArrayBuffer | ||
|
||
assert.strictEqual(Buffer.byteLength(sab), sab.byteLength, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex changed the style here.