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

[Implement] Naive Buffer.from, and friends #6

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[Implement] Naive Buffer.from, and friends #6

wants to merge 15 commits into from

Conversation

jtenner
Copy link
Contributor

@jtenner jtenner commented Jul 19, 2019

Implements Buffer.from<T>(value: T) and a few other convenience functions.

There are lots of obvious problems with not having the function overloads, but there are different paths we can take.

For instance, because there are no optional parameters, we must assume that the default encoding for strings is UTF8.

// utf-16 has a work around in AssemblyScript
let buff = Buffer.from(String.UTF16.encode(value));

// using Buffer.fromString(value, "utf16le");

Things to note about this function:

  • String[] objects do something complicated: string -> f64 -> u8 to remain compatible with node.
> Buffer.from(["3", "257.3", "15", "Infinity", "NaN"])
<Buffer 03 01 0F 00 00>
  • String objects need to be converted to UTF8
  • Buffer objects share their view of the same ArrayBuffer
  • ArrayBuffer objects are simply attached to a new Buffer
  • ArrayBufferView objects need to convert their values to u8.

@jtenner jtenner requested a review from dcodeIO July 19, 2019 18:24
@jtenner jtenner self-assigned this Jul 19, 2019
@jtenner jtenner added the enhancement New feature or request label Jul 19, 2019
@jtenner jtenner changed the title [Implement] Naive Buffer.from [Implement] Naive Buffer.from, and friends Aug 22, 2019
@jtenner
Copy link
Contributor Author

jtenner commented Aug 22, 2019

@RedDwarfian and @dcodeIO thoughts on this pull request?

@RedDwarfian
Copy link
Contributor

It is not clear in the Naive Buffer.from code where non-string arrays such as Array<u8> are handled. I would suggest adding a comment to clarify that.

@jtenner
Copy link
Contributor Author

jtenner commented Mar 13, 2020

We might like to pull the ASCII encoding pull request first. #27

Otherwise, this is ready for review.

if (i32(byteOffset < 0) | i32(byteOffset > buffer.byteLength - length)) throw new RangeError(E_INDEXOUTOFRANGE);
if (length == 0) return new Buffer(0);

return assembleBuffer(changetype<usize>(buffer), <usize>byteOffset, <u32>length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxGraey should this be min(length, buffer.byteLength - byteOffset)?

return assembleBuffer(changetype<usize>(buffer), 0, buffer.byteLength);
}

public static fromArray<T extends ArrayBufferView>(value: T, offset: i32 = 0, length: i32 = -1): Buffer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dcodeIO what is going to happen to this extends clause? Are we going to detach Array from ArrayBufferView?


// return and retain
return changetype<Buffer>(result);
// @ts-ignore: Buffer returns on all valid branches
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can suppress this error by returning changetype<Buffer>(null) at the bottom here after the ERROR. What is the right way to do this?

@jtenner jtenner requested a review from MaxGraey March 13, 2020 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants