-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[embind] Fix signature for 64 bit types. #22930
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
Conversation
Long longs were incorrectly being mapped to int for the signature. Fixes emscripten-core#22928
I should note when BIGINT is not enabled the |
}) | ||
def test_embind_bigint(self, args): | ||
self.do_runf('embind/test_bigint.cpp', '1000000000000n\n-1000000000000n', | ||
emcc_args=['-lembind', '-sWASM_BIGINT'] + args) |
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.
Wow I guess we just never had anyone try this before?
Why does this require -sWASM_BIGINT
to expose the issue? It looks like that lack of a long long
mapping is a problem regardless of this setting?
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.
It seems only ASYNCIFY exposed this because it uses the signature to create a DYNCALL with the legacy mode. Without asyncify bigints were working fine.
test/test_other.py
Outdated
'asyncify': [['-sASYNCIFY=1']] | ||
}) | ||
def test_embind_bigint(self, args): | ||
self.do_runf('embind/test_bigint.cpp', '1000000000000n\n-1000000000000n', |
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.
Is this new file missing from this PR?
Can you call it test_embind_bigint
to match the test name?
Should it be called test_embind_int64
perhaps? Or test_embind_long_long
?
} | ||
|
||
EMSCRIPTEN_BINDINGS(my_module) { | ||
emscripten::function("getBigInt", &getBigInt); |
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.
How about getUint64
? Or getLongLong
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.
lgtm % commentg
Long longs were incorrectly being mapped to int for the signature.
Fixes #22928