-
Notifications
You must be signed in to change notification settings - Fork 290
Fixed length list support #1992
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
Merged
alexcrichton
merged 27 commits into
bytecodealliance:main
from
cpetig:fixed-length-list
Apr 28, 2025
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
a5eb177
support fixed length lists, part 1
cpetig 6aeef59
more fixed size
cpetig c14d32d
fix tests for wit-parser
cpetig ef4b5e5
re-implement as two separate types (outside of WIT)
cpetig 5d2e4d8
adapt the tests for the new type
cpetig bd6db9a
handle reading correctly
cpetig 4b06d9d
wat parsing
cpetig c7b697c
generate fixed size lists in wit-smith
cpetig f1d6efe
proper rustfmt (whitespace change)
cpetig b5f54ee
implement Alex' suggetions, part one
cpetig 36e2bd8
implement roundtrip tests
cpetig 2abd0f8
limit the maximum size of a fixed list. to avoid memory explosion at …
cpetig 0b8d062
separate list types, as proposed by Alex
cpetig 4b30b54
post-rebase fixes
cpetig cca3733
fix the tests
cpetig 879702b
Move local test to CLI test
alexcrichton 1fc9e72
use same command line order as proposed by Alex
cpetig 0bfcb28
prefer the name chosen by Alex
cpetig e7b5952
apply missing changes by Alex (I should have fetched before rebase)
cpetig aa5618f
Merge remote-tracking branch 'origin/main' into fixed-length-list
cpetig ba0d39f
Merge branch 'main' of github.com:bytecodealliance/wasm-tools into fi…
cpetig bc48583
limit the size of flattened fixed size lists, forbid empty ones
cpetig 62fa3b7
test for zero and too high number
cpetig 1a6ac1e
check for type mismatch during composition
cpetig bf43b10
clearly separate list from fixed-size-list in code
cpetig b74e6d1
separate list types in encoder as well
cpetig 9669555
manually run rustfmt
cpetig File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Upon further reflection of this I'm starting to have second thoughts about this approach. This input for example:
when locally run as:
takes ~30 seconds to produce the result. I think the reason is that this case isn't hit and then the recursion gets to go hog wild for a bit.
Perhaps an alternative fix would be to iterate from 0 to
length
but then check each iteration of the loop if the max has been reached and bail out if so? That way each level would check the break-out and once that happens it'd quickly exit.Also if you're up for it I think that these would be good test cases to add to the test suite to ensure processing large or nested lists doesn't accidentally take forever
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.
Good catch, ... but the same problem shows with
I will take a look into whether I can shrink the time needed to process 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.
Oh that's a really good point! Ok in that case I'm going to go ahead and flag this for merge and follow-ups can handle fixes for cases like this.