-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Move default blocks into jump tables #5756
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
elliottt
merged 7 commits into
bytecodealliance:main
from
elliottt:trevor/default-in-jump-table
Feb 10, 2023
+115
−172
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a045a29
Move default blocks into jump tables
elliottt bb4cd8c
Preserve the old behavior of JumpTableData functions
elliottt 25127f1
Rework some uses of `push_entry`
elliottt f8c2a46
Fix deprecation version
elliottt dd61d04
Fix a warning
elliottt 71ce26c
Fix a missed function renaming
elliottt 8a34877
Comments from code review
elliottt 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ use serde::{Deserialize, Serialize}; | |
| /// Contents of a jump table. | ||
| /// | ||
| /// All jump tables use 0-based indexing and are densely populated. | ||
| /// | ||
| /// The default block for the jump table is stored as the last element of the underlying vector, | ||
| /// and is not included in the length of the jump table. It can be accessed through the | ||
| /// `default_block` function. The default block is iterated via the `iter` and `iter_mut` methods. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment should be updated to refer to |
||
| #[derive(Clone, PartialEq, Hash)] | ||
| #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))] | ||
| pub struct JumpTableData { | ||
|
|
@@ -22,72 +26,71 @@ pub struct JumpTableData { | |
| } | ||
|
|
||
| impl JumpTableData { | ||
| /// Create a new empty jump table. | ||
| pub fn new() -> Self { | ||
| Self { table: Vec::new() } | ||
| } | ||
|
|
||
| /// Create a new empty jump table with the specified capacity. | ||
| pub fn with_capacity(capacity: usize) -> Self { | ||
| Self { | ||
| table: Vec::with_capacity(capacity), | ||
| } | ||
| } | ||
| /// Create a new jump table with the provided blocks | ||
| pub fn with_blocks(table: Vec<Block>) -> Self { | ||
| pub fn new(def: Block, mut table: Vec<Block>) -> Self { | ||
| table.push(def); | ||
| Self { table } | ||
| } | ||
|
|
||
| /// Get the number of table entries. | ||
| pub fn len(&self) -> usize { | ||
| self.table.len() | ||
| /// Fetch the default block for this jump table. | ||
| pub fn default_block(&self) -> Block { | ||
| *self.table.last().unwrap() | ||
| } | ||
|
|
||
| /// Append a table entry. | ||
| pub fn push_entry(&mut self, dest: Block) { | ||
| self.table.push(dest) | ||
| /// Mutable access to the default block of this jump table. | ||
| pub fn default_block_mut(&mut self) -> &mut Block { | ||
| self.table.last_mut().unwrap() | ||
| } | ||
|
|
||
| /// Checks if any of the entries branch to `block`. | ||
| pub fn branches_to(&self, block: Block) -> bool { | ||
| self.table.iter().any(|target_block| *target_block == block) | ||
| /// The jump table and default block as a single slice. The default block will always be last. | ||
| pub fn all_branches(&self) -> &[Block] { | ||
| self.table.as_slice() | ||
| } | ||
|
|
||
| /// Access the whole table as a slice. | ||
| /// The jump table and default block as a single mutable slice. The default block will always | ||
| /// be last. | ||
| pub fn all_branches_mut(&mut self) -> &mut [Block] { | ||
| self.table.as_mut_slice() | ||
| } | ||
|
|
||
| /// Access the jump table as a slice. This excludes the default block. | ||
| pub fn as_slice(&self) -> &[Block] { | ||
| self.table.as_slice() | ||
| let last = self.table.len() - 1; | ||
| &self.table.as_slice()[0..last] | ||
| } | ||
|
|
||
| /// Access the whole table as a mutable slice. | ||
| /// Access the jump table as a mutable slice. This excludes the default block. | ||
| pub fn as_mut_slice(&mut self) -> &mut [Block] { | ||
| self.table.as_mut_slice() | ||
| let last = self.table.len() - 1; | ||
| &mut self.table.as_mut_slice()[0..last] | ||
| } | ||
|
|
||
| /// Returns an iterator over the table. | ||
| /// Returns an iterator to the jump table, excluding the default block. | ||
| #[deprecated(since = "7.0.0", note = "please use `.as_slice()` instead")] | ||
| pub fn iter(&self) -> Iter<Block> { | ||
| self.table.iter() | ||
| self.as_slice().iter() | ||
| } | ||
|
|
||
| /// Returns an iterator that allows modifying each value. | ||
| /// Returns an iterator that allows modifying each value, excluding the default block. | ||
| #[deprecated(since = "7.0.0", note = "please use `.as_mut_slice()` instead")] | ||
| pub fn iter_mut(&mut self) -> IterMut<Block> { | ||
| self.table.iter_mut() | ||
| self.as_mut_slice().iter_mut() | ||
| } | ||
|
|
||
| /// Clears all entries in this jump table. | ||
| /// Clears all entries in this jump table, except for the default block. | ||
| pub fn clear(&mut self) { | ||
| self.table.clear(); | ||
| self.table.drain(0..self.table.len() - 1); | ||
| } | ||
| } | ||
|
|
||
| impl Display for JumpTableData { | ||
| fn fmt(&self, fmt: &mut Formatter) -> fmt::Result { | ||
| write!(fmt, "[")?; | ||
| match self.table.first() { | ||
| None => (), | ||
| Some(first) => write!(fmt, "{}", first)?, | ||
| } | ||
| for block in self.table.iter().skip(1) { | ||
| write!(fmt, ", {}", block)?; | ||
| write!(fmt, "{}, [", self.default_block())?; | ||
| if let Some((first, rest)) = self.as_slice().split_first() { | ||
| write!(fmt, "{}", first)?; | ||
| for block in rest { | ||
| write!(fmt, ", {}", block)?; | ||
| } | ||
| } | ||
| write!(fmt, "]") | ||
| } | ||
|
|
@@ -102,31 +105,33 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn empty() { | ||
| let jt = JumpTableData::new(); | ||
| let def = Block::new(0); | ||
|
|
||
| let jt = JumpTableData::new(def, vec![]); | ||
|
|
||
| assert_eq!(jt.all_branches().get(0), Some(&def)); | ||
|
|
||
| assert_eq!(jt.as_slice().get(0), None); | ||
| assert_eq!(jt.as_slice().get(10), None); | ||
|
|
||
| assert_eq!(jt.to_string(), "[]"); | ||
| assert_eq!(jt.to_string(), "block0, []"); | ||
|
|
||
| let v = jt.as_slice(); | ||
| assert_eq!(v, []); | ||
| assert_eq!(jt.all_branches(), [def]); | ||
| assert_eq!(jt.as_slice(), []); | ||
| } | ||
|
|
||
| #[test] | ||
| fn insert() { | ||
| let def = Block::new(0); | ||
| let e1 = Block::new(1); | ||
| let e2 = Block::new(2); | ||
|
|
||
| let mut jt = JumpTableData::new(); | ||
|
|
||
| jt.push_entry(e1); | ||
| jt.push_entry(e2); | ||
| jt.push_entry(e1); | ||
| let jt = JumpTableData::new(def, vec![e1, e2, e1]); | ||
|
|
||
| assert_eq!(jt.to_string(), "[block1, block2, block1]"); | ||
| assert_eq!(jt.default_block(), def); | ||
| assert_eq!(jt.to_string(), "block0, [block1, block2, block1]"); | ||
|
|
||
| let v = jt.as_slice(); | ||
| assert_eq!(v, [e1, e2, e1]); | ||
| assert_eq!(jt.all_branches(), [e1, e2, e1, def]); | ||
| assert_eq!(jt.as_slice(), [e1, e2, e1]); | ||
| } | ||
| } | ||
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.
I was going to say that this should just use
table.all_branches()but then I remembered that would change the visit order. Instead, would you update the comment above to note that some callers depend on the default block being visited first?