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

added Atomics and SharedArrayBuffer #1463

Merged
merged 6 commits into from
Apr 26, 2019
Merged

added Atomics and SharedArrayBuffer #1463

merged 6 commits into from
Apr 26, 2019

Conversation

ibaryshnikov
Copy link
Member

@ibaryshnikov ibaryshnikov commented Apr 15, 2019

Added Atomics and SharedArrayBuffer to js-sys.
Ready for review.

Closes #1443

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good! I wonder if this could take a leaf from types like JSON to and have a mod Atomics { ... } instead of a type Atomics?

crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
crates/js-sys/src/lib.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Contributor

Looking great @ibaryshnikov!

Thinking some more on this, what do you think of changing f64 to i32? It looks like it's documented that you can only pass integer arrays up to 32 bits, so using i32 may actually cover everything here and may be easier to use than dealing with floats (and perhaps faster since floats sometimes have a bit of a cost to them).

@ibaryshnikov
Copy link
Member Author

Oh, I didn't notice it in the beginning. I thought that f32 and f64 typed arrays were allowed for operations like add. Using i32 looks fine to me. What about u32?

@alexcrichton
Copy link
Contributor

Both u32 and i32 work but i32 doesn't need a job shim to switch the number to unsigned, so I think I'd lean towards i32 for this

@ibaryshnikov
Copy link
Member Author

Fine. I also need to change static_method_of = Atomics to js_namespace = Atomics after changing from pub type Atomics to pub mod Atomics, right?

@alexcrichton
Copy link
Contributor

That sounds right yeah, I sort of forget the specifics but if it works it works!

…d_of to js_namespace, set typed_array type to Int32Array in notify and wait methods
Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Did you still have changes you wanted to make or is this good to go?

@ibaryshnikov
Copy link
Member Author

I'd like to write tests, but I can do it in a separate pr, so we can merge this I think.

@ibaryshnikov ibaryshnikov changed the title [WIP] added Atomics and SharedArrayBuffer added Atomics and SharedArrayBuffer Apr 26, 2019
@alexcrichton alexcrichton merged commit 7e512ba into rustwasm:master Apr 26, 2019
@ibaryshnikov
Copy link
Member Author

@alexcrichton thank you for very useful comments!

@ibaryshnikov ibaryshnikov deleted the atomics-support branch April 26, 2019 04:27
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.

Atomics and SharedArrayBuffer support
2 participants