Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
More support for wasm #178
base: master
Are you sure you want to change the base?
More support for wasm #178
Changes from 15 commits
68e6af1
e7d9517
2df83f9
3af9272
e09f999
88f1cc4
9539b5f
bac32e9
1a620f4
45ff7e3
b44ce75
4e8e9de
7856777
9d36a73
2d3497d
e4e0e06
d27cadf
aeeb3cc
77991b6
b9f7954
4efab3b
69aa772
bd9425a
d293552
4b13a9a
33aa079
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Let's avoid calling
.toDart
twice, in wasm mode it'll create a wrapper, there is no need to create the wrapper twice.See: https://api.dart.dev/dart-js_interop/JSUint8ArrayToUint8List/toDart.html
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.
Why are we changing behavior here?
Is this deliberate? If so should it not be orthogonal to this PR, and happen in a separate PR?
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.
I noticed that when generating unsupported lists (e.g.
Uint64List
) in dart2js, an UnsupportedError is thrown. For example, the following test succeeds.The test fails in dart2wasm.
There is no JS class corresponding to
Uint64List
. Therefore, whenfillRandomBytes(Uint64List(32))
is called in dart2wasm, the process reaches this code.I changed it to UnsupportedError to keep the process consistent between dart2js and dart2wasm. It is possible to split a PR.
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.
I added test case. 4efab3b
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.
And restore previous code. The addition of the test makes it easier to understand the scope of the impact. bd9425a
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.
I'm curious, why do we need to skip this?
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.
This is because with dart2wasm, the value is not reflected in the UInt8List. All
data
values are 0.