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

Add get/set for TypedArrays. #2001

Merged
merged 3 commits into from
Feb 18, 2020
Merged

Conversation

richard-uk1
Copy link
Contributor

This provides the ability to do

let value = array[idx];
array[idx] = value;

for typed arrays. They are named with an underscore so thay do not clash with the set method, which does something different.

@alexcrichton
Copy link
Contributor

Thanks for the PR! This feels a bit odd though with the trailing underscores? Is there perhaps a different way we could expose this?

@richard-uk1
Copy link
Contributor Author

Hmm, looking at the mdn page for indexed collections, it seems that currently the two main types for this are Array and the various typed arrays. So we can either bikeshed some names like get_array or get_indexed etc., or have a trait called IndexedArray (bikesheddable) with methods get and set. The js_sys::Array type just has the get and set methods already defined, but we can't do that on typed arrays because the set method means something else. Unfortunately, the typed arrays don't inherit from Array, so we can't just deref to Array either.

I don't think there is an obvious optimal solution, but I think personally I'd go for something like get_array_like, with documentation explaining the situation (e.g. saying

equivalent to val = arr[idx] on the javascript side

). I'll implement this in the PR, but am happy to change it to something else if preferred.

@Pauan
Copy link
Contributor

Pauan commented Feb 18, 2020

I'd be in favor of get_index and set_index.

@richard-uk1
Copy link
Contributor Author

Actually thinking about it again, I agree.

@alexcrichton alexcrichton merged commit 7db01a7 into rustwasm:master Feb 18, 2020
@alexcrichton
Copy link
Contributor

Thanks!

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.

3 participants