Skip to content
This repository has been archived by the owner on Nov 12, 2022. It is now read-only.

Modify TypedArray::create API to accept either a length or an initial slice #336

Closed
jdm opened this issue Feb 9, 2017 · 5 comments
Closed

Comments

@jdm
Copy link
Member

jdm commented Feb 9, 2017

For cases when we create a new typed array with an initial slice, we always want to use the length of the provided slice. Passing the same length as another argument is redundant; we should use a custom enum argument instead (eg. CreateWith::Length(5) and CreateWith::Slice(foo)).

Code: src/typedarray.rs
Tests: cargo test --name typedarray (tests/typedarray.rs)

@jdm jdm added the E-easy label Feb 9, 2017
@simon-whitehead
Copy link
Contributor

simon-whitehead commented Feb 9, 2017

This looks like a great first PR. Mind if I give it a shot? I may need some guidance around code/test style but hopefully I won't need too much hand-holding (I can read and write Rust .. I just haven't contributed to Servo yet).

@jdm
Copy link
Member Author

jdm commented Feb 9, 2017

Please do!

@jdm jdm added the C-assigned label Feb 9, 2017
@simon-whitehead
Copy link
Contributor

simon-whitehead commented Feb 11, 2017

@jdm Are you able to give me some guidance/documentation around versioning? I imagine I'll be required to bump the crate version? How does that affect how this crate version is applied to servo later? I'm just curious given I'll be changing a function signature that may have ripple effects for consumers.

@jdm
Copy link
Member Author

jdm commented Feb 11, 2017

We use a git dependency for rust-mozjs in Servo, so the crate version doesn't actually matter for that case. This crate is also not published to crates.io, so we generally have not been updating the version.

bors-servo pushed a commit that referenced this issue 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 -->
@KiChjang
Copy link
Contributor

Fixed by #337.

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

No branches or pull requests

3 participants