Add val_cnt and pop_cnt in side table#2
Conversation
zhouwfang
changed the title
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
Thanks! I think the main changes would be:
self.stitch()needs to takeval_cntself.branch()doesn't take any argument and computestackfrom the total number of values in the labels.
| struct SideTableBranch<'m> { | ||
| parser: &'m [u8], | ||
| side_table: usize, | ||
| val_cnt: u32, |
| parser: &'m [u8], | ||
| side_table: usize, | ||
| val_cnt: u32, | ||
| pop_cnt: u32, |
There was a problem hiding this comment.
This should probably be something like
| pop_cnt: u32, | |
| stack: usize, |
And be initialized by stack.len() (like side_table is initialized with side_table.len()).
We probably need a function to compute the stack length (going through all labels). We don't need to compute across functions because we can only jump within a function. We could try to memoize the sum of previous label in each label.
There was a problem hiding this comment.
Is the stack similar to the Label::values_cnt in exec.rs?
There was a problem hiding this comment.
Yes, that's the value stack. There's one difference though, is that in exec.rs it's the value stack of the thread (i.e. function and label stack). Here we only need the value stack of the function (i.e. label stack).
| @@ -793,45 +805,52 @@ impl<'a, 'm> Expr<'a, 'm> { | |||
| } | |||
|
|
|||
| fn stitch(&mut self, source: SideTableBranch, target: SideTableBranch) { | |||
There was a problem hiding this comment.
We probably need to take val_cnt as argument.
| val_cnt: 0, | ||
| pop_cnt: 0, | ||
| val_cnt, | ||
| pop_cnt, |
There was a problem hiding this comment.
I think this should just be (source.stack - target.stack) as u32 or something like that. We should actually check that the subtraction and the cast are not overflowing, and return Unsupported otherwise.
We should probably also remove val_cnt from source.stack (again checking for overflow).
d9d837a to
a9318f6
Compare
google#46 --------- Co-authored-by: Zhou Fang <[email protected]> Co-authored-by: Julien Cretin <[email protected]>
Part of google#448 and google#208. --------- Co-authored-by: Zachary Vander Velden <[email protected]>
a9318f6 to
39dfe61
Compare
39dfe61 to
6890972
Compare
There was an earlier review in zhouwfang#2. #46 --------- Co-authored-by: Julien Cretin <[email protected]> Co-authored-by: Zhou Fang <[email protected]>
2 participants
I follow
wasefire/crates/interpreter/src/exec.rs
Lines 1039 to 1047 in 15b120f
google#46