Skip to content

Conversation

zhouwfang
Copy link
Contributor

@zhouwfang zhouwfang commented Mar 7, 2025

#46

@zhouwfang zhouwfang requested a review from ia0 as a code owner March 7, 2025 06:10
@zhouwfang zhouwfang changed the base branch from main to dev/fast-interp March 7, 2025 06:10
@zhouwfang zhouwfang marked this pull request as draft March 7, 2025 06:10
delta_ip: (parse_u16(&self.0, 0) as i16) as i32,
delta_stp: (parse_u16(&self.0, 2) as i16) as i32,
delta_ip: (parse_u16::<Use>(&self.0, 0).unwrap() as i16) as i32,
delta_stp: (parse_u16::<Use>(&self.0, 2).unwrap() as i16) as i32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we want this function to be parametric over the mode. We use it in patch_branch() during verification. And actually, we should not panic when indexing with [source.branch_table] there. We should also use get() (unless we have a test somewhere else).

Copy link
Contributor Author

@zhouwfang zhouwfang Mar 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point that it should be generic too. But where is the "indexing" in this function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no "indexing" in this function. The indexing I mentioned was where it is called in valid.rs. That can be done in the next PR. I hope I'll be able to find out all those issues in the final review from dev/fast-interp to main. There are actually many places where we would panic instead of returning an error on user errors.

@zhouwfang zhouwfang changed the title [Draft] Make metadata() generic Make metadata() generic Mar 8, 2025
@zhouwfang zhouwfang changed the title Make metadata() generic Make metadata() and view() generic Mar 8, 2025
@zhouwfang zhouwfang changed the title Make metadata() and view() generic Make metadata(), parse_u16() and view() generic Mar 8, 2025
@zhouwfang zhouwfang marked this pull request as ready for review March 8, 2025 00:31
@zhouwfang zhouwfang requested a review from ia0 March 8, 2025 00:31
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM. We can do type_idx and branch_table in another PR since this one is good.

@@ -42,7 +43,7 @@ pub struct Metadata<'m>(&'m [u8]);

impl<'m> Metadata<'m> {
pub fn type_idx(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type_idx and branch_table also need to be polymorphic, because they are called from valid.rs too. But this can be done in another PR.

@ia0 ia0 merged commit c89333b into google:dev/fast-interp Mar 10, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants