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

TypedArray::create API accept length or slice #337

Merged
merged 1 commit into from Feb 11, 2017

Conversation

@simon-whitehead
Copy link
Contributor

simon-whitehead commented Feb 11, 2017

Issue: #336

Changes TypedArray::create signature to accept a new CreateWith argument in place of the length and data arguments.

Includes similar tests to what was already there using the new enum.

Some notes:

  • My first Servo PR. Yay!
  • Rustfmt seems to have made some changes. Not sure if thats a problem. The license /* */ block comments are now // single line comments. And some other multi-line code has been changed to single line code. If this is an issue let me know and I'll disable rustfmt and revert those changes back.
  • I didn't bump the crate version. I am not sure about the process around that and looking at other PRs I've noticed none of those bumped the version number either.
  • I don't yet understand how this applies to Servo. From what I can see everything uses the macro and not TypedArray::create directly. I may have missed something though.

Feedback welcome.

@jdm - tagging you since you opened the issue (I hope thats okay?)


This change is Reviewable

@simon-whitehead
Copy link
Contributor Author

simon-whitehead commented Feb 11, 2017

Also one other potential noteworthy change.

The original tests allowed different length and data length inputs. For example, create with a length of 10 but a slice with length 5 would result in a 0 padded slice with a length of 10.

The issue itself seems to suggest this shouldn't be possible and so after the change was made, this situation wasn't possible and I have updated the existing tests to reflect that.

@jdm
Copy link
Member

jdm commented Feb 11, 2017

There are uses of ArrayBuffer::create and Uint8ClampedArray::Create and in Servo right now, and servo/servo#15427 adds more typed array creation.

@jdm
Copy link
Member

jdm commented Feb 11, 2017

This looks great! Thank you for working on this!
@bors-servo: r+

@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2017

📌 Commit 86f06e4 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2017

Testing commit 86f06e4 with merge 901b1b5...

bors-servo added a commit that referenced this pull request Feb 11, 2017
TypedArray::create API accept length or slice

Issue: #336

Changes `TypedArray::create` signature to accept a new `CreateWith` argument in place of the `length` and `data` arguments.

Includes similar tests to what was already there using the new enum.

Some notes:

* My first Servo PR. Yay!
* Rustfmt seems to have made some changes. Not sure if thats a problem. The license `/* */` block comments are now `//` single line comments. And some other multi-line code has been changed to single line code. If this is an issue let me know and I'll disable rustfmt and revert those changes back.
* I didn't bump the crate version. I am not sure about the process around that and looking at other PRs I've noticed none of those bumped the version number either.
* I don't yet understand how this applies to Servo. From what I can see everything uses the macro and not `TypedArray::create` directly. I may have missed something though.

Feedback welcome.

@jdm - tagging you since you opened the issue (I hope thats okay?)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/337)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 901b1b5 to master...

@bors-servo bors-servo merged commit 86f06e4 into servo:master Feb 11, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@simon-whitehead
Copy link
Contributor Author

simon-whitehead commented Feb 11, 2017

No problem! Thanks for merging so quickly! 😃

(Ps: this was such a painless process. I will look out for more issues I can help with 👍)

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.