-
-
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
Allow i64 values to be passed #35
Comments
Seems plausible to me! I initially hesitated in exposing this because I wasn't sure what types these would map to on the JS side of things, but maybe long.js is standard enough that "pulling in a dependency to do this" isn't so bad? |
Long.js doesn't everything we need and is fast (uses WebAssembly when possible), it's ok to pull in that dependency IMO.
Well i64 can not be represented as a primitive in JavaScript, you'll need to use an object (like Long.js does). It will also hide the implementation (long.js or bigint). |
Oh how would the implementation be hidden? Wouldn't JS want to pass values in (aka construct them) and also read the results? |
My point is that we can't represent a i64 value is JS so you couldn't pass it in the constructor, you could either as string or as a two's complement int. To be more accurate, we need an abstraction to represent the value. For example:
Apart with BigInt all built-in operations won't work ( Depending how far you want to go but we have a PR against Babel to implement Does that make sense to you? |
With WebAssembly/design#1186 being created pretty recently, I wonder if it'd be best to hold off on a long.js polyfill and wait for |
That's a good question. I think it could act as a good experimentation and make i64 available until BigInt is there, what do you think? |
Ah ok yeah, if it's that far off then doing the polyfill of long.js for now seems reasonable to me! |
I think we could mutualize some work here. I would like to use the same mechanism in multiple projects, what do you think if I publish a The logic would be
|
Hm I think unfortunately I'm not familiar enough with idiomatic JS really to make a judgement call here. I'd naively expect that a custom package for this wouldn't be idiomatic, but I also don't know the idioms so I'm not sure. |
I'm not sure to fully understand your concern. The package is just here to provide an abstraction, with the same API than BigInt. Ideally for user it's transparent. |
I'd agree with @alexcrichton, automatically pulling an external dependency is a concerning decision since now you need to be either responsible for keeping it up-to-date or forcing user to do it, either way keeping code locked to such dependency. On the other hand, there are no really existing idioms for 64-bit integers in JS yet because they just weren't part of it. Most common was to either pass them as string or loosely convert to floats (then you preserve precision up to 53 bits), but each way has its own pitfalls. So my suggestion would be to go the way each new API in JS gets adopted: generate calls to upcoming native |
I would prefer the two's complement because:
Some operations are tricky to implement on two i32 but hopefully for us Long.js already done that.
I like that idea, we could provide a polyfill (using long.js or whatever), checking first for
I might have misworded that. My idea is that we wouldn't expose/impose anything to the user, the user just gets an object with the BigInt API (backend by BigInt or not). The package simplifies the creation of that object, just like a factory. Or the package would contain the polyfill of BigInt (backend by BigInt or not). Does that make sense to you? |
Oh ok. It did initially sound like you want to include specifically long.js together with library. So we agree that wasm-bindgen can just use |
Delegating to proposed |
That sounds good but unless we're adding a transpilation phase we won't be able to use the built-in operators (+, -, /...) where BigInt will. |
@xtuc Why would we need to use these operators in bindings code? We only need to construct BigInt instances, and it's up to the user of bindings how to use them, whether to transpile their code etc. |
For example: someBindingToWasm.exports.getMaxI64() - 1n should work according to the I mentioned transpilation because that could be a possible solution. |
But, again, what you posted would be the user code, not the generated one, no? So it's outside of wasm-bindgen's responsibility in any way and it's up to the user to choose how they want to work with BigInts. |
I agree, but my point is that if you want to add two numbers:
As a user you can't use the same code. |
I understand what you're saying but I don't understand how it's related or how it affects wasm-bindgen :) |
I mean, as long as we just use |
Ok, I get it now, you're right! |
FYI: Native https://developers.google.com/web/updates/2018/05/bigint Viewing wasm memory as a |
I've sent a PR to take advantage of the new support in Chrome beta at #188 |
is there any polyfill available to work with existing browser? |
Unfortunately I'm not currently aware of one myself |
Not in Babel. It requires changing somehow the built-in operations. |
We are working on the conversion between i64 and JS's BigInt; instead of the current polyfilling strategy it could be exposed natively. However, in order to move the proposal forward (WebAssembly/JS-BigInt-integration#15) we would need some toolchain support. I think it would be great to implement something here, behind an experimental flag. |
I mentioned here WebAssembly/design#1172 that we will be able to use the
BigInt
proposal to represent the i64 values from WebAssembly.Currently there is not support for BigInt in browsers (nor in Babel). So I believe we can use this kind of tools to allow it.
We use a 64 bit two's-complement (https://github.com/dcodeIO/long.js).
Have you considered to implement it?
Edit:
I just remembered that WebAssembly doesn't allow multiple results for a func, I'm not sure what's the best solution, maybe the linear memory?
When I said "we use" I meant in https://github.com/xtuc/webassemblyjs which is not the same use-case.
The text was updated successfully, but these errors were encountered: