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
Draft implementation of wasm shaper #122
Draft implementation of wasm shaper #122
Changes from 23 commits
6053e0b
40a8cf8
8deb543
def1128
5160985
aaef802
f080e29
4865ba0
d5ef3cf
ac25191
2202d8b
57da6f0
06ae9dd
ef0b838
4c3595d
9b0809c
b535177
230ea71
82929f8
b0845a5
7a0da1f
77133bd
1da938f
11a062b
84b9f58
234ef56
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.
Shouldn't we check for the
Wasm
table here? I.e. this should be a runtime decision, not compile-time one.If we have
Wasm
table, tryshape_with_wasm
, otherwiseshape_with_plan
.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 what this code does. The
Wasm
table is the first thingshape_with_wasm
checks for, and if it is not there it returnsNone
. The compile time check here is for the featuree asshape_with_wasm
does not exist then.ِEdit: I moved it to inside
shape_with_plan
.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.
But if there is no
Wasm
table you exit the shaping immediately, instead of calling the regular shaper. No?I would expect something like:
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.
yeah
shape_with_wasm
fails if there is no"Wasm"
table. The fallback to regular shaper happens inshape_with_plan
here. You can see the regular tests pass just fine with the feature turned on.