-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 JsString <-> char conversions #1473
Conversation
Seems reasonable to me, thanks @RReverser! Just to confirm with a second opinion, @Pauan mind checking as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, just a couple things need to be fixed.
@Pauan All your other suggestions make sense to me. Any chance you could submit them using "Github suggestion" comments which I could easily accept (including from the phone)? Otherwise I won't get around to this until after the Easter holiday (later next week). |
7f78a25
to
91f112d
Compare
These are pretty common and already supported via ABI conversions, yet pretty easy to get wrong when converting them manually. Fixes rustwasm#1363.
Ok fixed most nits (except the |
I think I would agree with @Pauan here actually, could the |
Hmm, |
Sorry for the delay, I recently caught a nasty cold. I'm a bit wary of I feel like I think they do seem rather useful, since JS itself doesn't make a distinction between strings and chars. But I'm also okay with deferring them to a future PR. |
No worries and get well! |
This seems to spark controversy, so removing for now but should be easy enough to still add in the future.
Alright, I removed |
Thanks @RReverser! |
These are pretty common and already supported via ABI conversions, yet pretty easy to get wrong when converting them manually.
Fixes #1363.