Skip to content

Bindings for TypedArray subclasses #486

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

Merged
merged 2 commits into from
Jul 17, 2018
Merged

Conversation

cdisselkoen
Copy link
Contributor

More progress towards #275. This exposes the bindings for constructors and fill methods for Uint16Array, Uint32Array, Uint8ClampedArray, Int8Array, Int16Array, Int32Array, Float32Array, and Float64Array, following very closely the already-existing support for Uint8Array supplied in #316. Small tweaks to the existing Uint8Array code (moved to alphabetical position in js.rs, refactored tests) are included in this PR.

Feedback welcome - this is my first contribution to this project, and actually to any project written in the Rust language.

@cdisselkoen
Copy link
Contributor Author

Rebased on master to fix merge conflicts and fix compatibility with latest Nightly Rust

src/js.rs Outdated
///
/// http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/fill
#[wasm_bindgen(method)]
pub fn fill(this: &Float32Array, value: JsValue, start: u32, end: u32) -> Float32Array;
Copy link
Contributor

Choose a reason for hiding this comment

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

This was previously JsValue here but I wonder if this would be better bound as f32? (or the appropriate type on each array below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems sensible to me. Perhaps the JsValue was there only because that's the type on Array.fill()

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Jul 17, 2018
Not a lot of attention has been paid to dealing with conflicts of symbols
between crates and different `#[wasm_bindgen]` blocks. This commit starts to fix
this issue by unblocking rustwasm#486 which first ran into this. Currently there's a bug
where if two independent crates bind the same JS API they'll generate the same
symbol which causes conflicts for things like LTO or linking in general.

This commit starts to add a "salt" to all symbols generated by `wasm-bindgen`
(these are all transparent to the user) to ensure that each crate's invocations
are kept apart from one another and using the correct bindings.
@alexcrichton
Copy link
Contributor

Ah it unfortunately looks like this is running into a few issues. One is #496 which fixes the underlying bug here causing the test to fail. The other is an upstream bug for rust-lang/rust about the bad error message here. Once #496 is merged we should be able to merge this!

@alexcrichton
Copy link
Contributor

rust-lang/rust#52474 is the accompanying Rust PR to fix the diagnostic here

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Jul 17, 2018
Not a lot of attention has been paid to dealing with conflicts of symbols
between crates and different `#[wasm_bindgen]` blocks. This commit starts to fix
this issue by unblocking rustwasm#486 which first ran into this. Currently there's a bug
where if two independent crates bind the same JS API they'll generate the same
symbol which causes conflicts for things like LTO or linking in general.

This commit starts to add a "salt" to all symbols generated by `wasm-bindgen`
(these are all transparent to the user) to ensure that each crate's invocations
are kept apart from one another and using the correct bindings.
alexcrichton added a commit that referenced this pull request Jul 17, 2018
Not a lot of attention has been paid to dealing with conflicts of symbols
between crates and different `#[wasm_bindgen]` blocks. This commit starts to fix
this issue by unblocking #486 which first ran into this. Currently there's a bug
where if two independent crates bind the same JS API they'll generate the same
symbol which causes conflicts for things like LTO or linking in general.

This commit starts to add a "salt" to all symbols generated by `wasm-bindgen`
(these are all transparent to the user) to ensure that each crate's invocations
are kept apart from one another and using the correct bindings.
@alexcrichton alexcrichton merged commit a05d930 into rustwasm:master Jul 17, 2018
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.

2 participants