Add int64_t support to embind using bigint support by cuberoot · Pull Request #13889 · emscripten-core/emscripten

cuberoot · 2021-04-13T21:10:43Z

This PR adds int64_t and uint64_t support to embind when the -s WASM_BIGINT flag is used (see issue 11726).

Emscripten has support for handling int64_t and uint64_t as JS bigint values, but embind does not know anything about those values.

This PR is incomplete. I need to add tests and work out how to add guards so that this change does not cause issues when the -s WASM_BIGINT is not used.

welcome · 2021-04-13T21:10:45Z

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

kripken

Nice work, thanks @cuberoot ! I think this is correct.

Please add testing for this, I think otherwise it looks good.

@RReverser maybe you want to take a look too?

src/embind/embind.js

RReverser

Looks good to me, thanks!

* add range check and numeric conversions to toWireType implementation for int64_t and uint64_t * add basic test for emscripten::val and 64-bit bigints * fix indentation issue

…mscripten into cuberoot/embind-int64

cuberoot · 2021-04-15T07:30:42Z

src/embind/embind.js

+
+    var isUnsignedType = (name.indexOf('u') != -1);
+
+    // maxRange comes through as -1 for uint64_t (see issue 13902). Work around that temporarily


See issue #13902

src/embind/embind.js

cuberoot · 2021-04-15T07:35:44Z

I added more intelligence to the toWireType, so check that out.

I will add more tests for binding function calls tomorrow.

src/embind/embind.js

- Fix issue with `val::as<int64_t>()` not working - Add more tests for `val` and embind with `int64_t` and `uint64_t` - Fix failing tests using suggestion from Kripken

system/include/emscripten/val.h

…into cuberoot/embind-int64-wip

kripken · 2021-04-28T15:35:22Z

system/include/emscripten/val.h

            } w[2];
            double d;
+            int64_t i;
+            uint64_t u;


do we need both signed and unsigned? i'd hope only unsigned is enough, as with unsigned u above for 32-bit values.

@kripken You are correct, the int64_t member was redundant.

@kripken

As per suggestion by @kripken in PR

kripken

Thanks!

This is the next step in refactorings I started back in emscripten-core#20383 to merge all Embind generic method callers into a single mechanism. I never got around to landing this PR because I kept trying to split it into even smaller changes / steps, never found a good way to do so due to how entangled they were, got frustrated and left it alone. However, aside from being a nice-to-have optimisation or cleanup refactoring, it's also an easy way to deal with issues like emscripten-core#24547 which makes it a bit more pressing. Without this PR, we'd have to duplicate the `emval_as`+`emval_as_int64`+`emval_as_uint64` fix from emscripten-core#13889 for `emval_call` and `emval_call_method` as well, but after this PR everything goes through the same centralised implementation where we can deal with all Embind types in a consistent manner. There are some more things that I'd like to clean up here (including some of the complexity added by this very own PR), but in order to try and keep review scope smaller, I'm going to submit them separately. Fixes emscripten-core#24547.

This is the next step in refactorings I started back in #20383 to merge all Embind generic method callers into a single mechanism. I never got around to landing this PR because I kept trying to split it into even smaller changes / steps, never found a good way to do so due to how entangled they were, got frustrated and left it alone. However, aside from being a nice-to-have optimisation or cleanup refactoring, it's also an easy way to deal with issues like #24547 which makes it a bit more pressing. Without this PR, we'd have to duplicate the `emval_as`+`emval_as_int64`+`emval_as_uint64` fix from #13889 for `emval_call` and `emval_call_method` as well, but after this PR everything goes through the same centralised implementation where we can deal with all Embind types in a consistent manner. There are some more things that I'd like to clean up here (including some of the complexity added by this very own PR), but in order to try and keep review scope smaller, I'm going to submit them separately. Fixes #24547.

Add int64_t support to embind using bigint support

36220b6

Merge branch 'main' into cuberoot/embind-int64

3112759

kripken reviewed Apr 14, 2021

View reviewed changes

src/embind/embind.js Outdated Show resolved Hide resolved

RReverser approved these changes Apr 14, 2021

View reviewed changes

cuberoot added 2 commits April 15, 2021 19:28

Improve toWireType inplementation for bigint and 64bit

fde8c15

* add range check and numeric conversions to toWireType implementation for int64_t and uint64_t * add basic test for emscripten::val and 64-bit bigints * fix indentation issue

Merge branch 'cuberoot/embind-int64' of https://github.com/cuberoot/e…

bd842ba

…mscripten into cuberoot/embind-int64

cuberoot commented Apr 15, 2021

View reviewed changes

src/embind/embind.js Outdated Show resolved Hide resolved

kripken reviewed Apr 19, 2021

View reviewed changes

src/embind/embind.js Outdated Show resolved Hide resolved

src/embind/embind.js Outdated Show resolved Hide resolved

Fix emscripten::val for int64 and add more tests

f90457b

- Fix issue with `val::as<int64_t>()` not working - Add more tests for `val` and embind with `int64_t` and `uint64_t` - Fix failing tests using suggestion from Kripken

cuberoot commented Apr 20, 2021

View reviewed changes

system/include/emscripten/val.h Show resolved Hide resolved

cuberoot added 3 commits April 20, 2021 20:30

Merge branch 'main' into cuberoot/embind-int64-wip

07794e3

Merge branch 'main' of https://github.com/emscripten-core/emscripten …

65d3f21

…into cuberoot/embind-int64-wip

Fix copyright

74f8eef

kripken reviewed Apr 28, 2021

View reviewed changes

cuberoot added 2 commits May 3, 2021 12:57

GenericWireType only needs uint64_t

ca8f4e9

As per suggestion by @kripken in PR

Merge branch 'main' into cuberoot/embind-int64

a519f9f

kripken approved these changes May 5, 2021

View reviewed changes

kripken merged commit cf9f2c9 into emscripten-core:main May 5, 2021

cuberoot deleted the cuberoot/embind-int64 branch May 11, 2021 22:03

kripken mentioned this pull request Jun 8, 2021

Add support for unsigned long long #11726

Closed

RReverser mentioned this pull request Jun 16, 2025

[embind] Use a single invoker mechanism #24577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add int64_t support to embind using bigint support#13889

Add int64_t support to embind using bigint support#13889
kripken merged 10 commits intoemscripten-core:mainfrom
cuberoot:cuberoot/embind-int64

cuberoot commented Apr 13, 2021

Uh oh!

welcome bot commented Apr 13, 2021

Uh oh!

kripken left a comment

Uh oh!

Uh oh!

RReverser left a comment

Uh oh!

cuberoot Apr 15, 2021

Uh oh!

Uh oh!

cuberoot commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken Apr 28, 2021

Uh oh!

cuberoot May 3, 2021

Uh oh!

kripken left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		var isUnsignedType = (name.indexOf('u') != -1);

		// maxRange comes through as -1 for uint64_t (see issue 13902). Work around that temporarily

Conversation

cuberoot commented Apr 13, 2021

Uh oh!

welcome bot commented Apr 13, 2021

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RReverser left a comment

Choose a reason for hiding this comment

Uh oh!

cuberoot Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cuberoot commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kripken Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

cuberoot May 3, 2021

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants