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

Adding in Reflect::get_f64, Reflect::get_u32, Reflect::set_f64, and Reflect::set_u32 #1225

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Feb 4, 2019

It is quite common to want to get/set numeric properties (e.g. on Arrays, TypedArrays, etc.)

These methods are both more convenient and faster than Reflect::get and Reflect::set.

It might also be worth it to add methods directly to some of the types (e.g. Array and the TypedArrays), but that has some implementation difficulties which we should discuss first.

@Pauan
Copy link
Contributor Author

Pauan commented Feb 4, 2019

These are the "implementation difficulties":

  • Reflect.get doesn't work on JS strings, but foo[bar] does. So we really want something more generic than Reflect.get (e.g. a wasm-bindgen built-in which just does foo[bar]).

  • Since we know in many situations that exceptions cannot be thrown, it would be nice if we could avoid catch, but that implies that we'll need two methods: a non-catching method and a catching method. I'm not sure what to name them, or how to organize them in the cleanest way.

  • In the case of typed arrays, they already have a set method, however that set method is completely different from Reflect::set, so I'm not sure how to resolve that.

@alexcrichton
Copy link
Contributor

Thanks! This looks good to me to merge for sure. I wonder if we would want to develop a convention for indexing get/set methods? We could add them as conveniences to typed arrays as well as strings perhaps.

For exceptions in the long-term we won't have try/catch at all in theory when we have exception handling in wasm which I think should be much faster. In the meantime though I'm not sure how to handle this. Are you worried about the performance? Or moreso the code size impact on JS?

@Pauan
Copy link
Contributor Author

Pauan commented Feb 6, 2019

Are you worried about the performance? Or moreso the code size impact on JS?

I'm concerned about three things:

  1. Raw performance, since try/catch imposes some performance cost (and exceptions in wasm might not be completely free either).

  2. Code size, though this is perhaps less important than raw performance.

  3. It shouldn't need to return Result (in the situations where it cannot error), but currently it always returns Result, and using unwrap has its own share of issues.

@alexcrichton
Copy link
Contributor

Ok makes sense! I think though that all those worries are handled by the future wasm exception handling proposal? In that proposal we wouldn't ever return Result in theory and there'd be a special API to actually catch a JS exception object presumably. Additionally in that theoretical future there's no try/catch nor no unwrap, the only major impact is that throwing an exception is likely to be slow, but that's all expected in Rust

@Pauan
Copy link
Contributor Author

Pauan commented Feb 11, 2019

In that proposal we wouldn't ever return Result in theory and there'd be a special API to actually catch a JS exception object presumably.

That sounds reasonable, but how long will it take until that proposal is specced and implemented? It'd be nice to have something which works now, rather than years in the future.

In any case, all of that is orthogonal to this PR, so we can discuss it someplace else.

@alexcrichton
Copy link
Contributor

Ok I'll go ahead and merge this then. FWIW exceptions are probably years out, although they won't change functionality really just code size in some situations.

@alexcrichton alexcrichton merged commit c49b87b into rustwasm:master Feb 13, 2019
@Pauan Pauan deleted the get_index branch February 14, 2019 04:42
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.

None yet

2 participants