Clean up after 'WIP: integration test (#368)' #603
Conversation
matthiasgoergens
force-pushed
the
matthias/clean-up-after-368
branch
from
032d82a
to
4c4fa95
Compare
| @@ -35,6 +35,24 @@ struct Args { | |||
| max_steps: Option<usize>, | |||
| } | |||
|
|
|||
| fn with_panic_hook<F, R>(hook: Box<dyn Fn(&PanicHookInfo<'_>) + Sync + Send + 'static>, f: F) -> R | |||
There was a problem hiding this comment.
I feel this is a bit cleaner for the reader to extract, then inlining it into the code further down.
There was a problem hiding this comment.
Might as well go to the root cause and change the assert to a returned error.
There was a problem hiding this comment.
Interesting suggestion. Can you make a PR for that, please?
I didn't want to change the behaviour here, just conservatively make the code that we have easier to understand.
|
seems ok. How did you find these? reading codes? cargo clippy? |
The main goal for me was reading through the code of #368 to understand what's going on. These small improvements are just a by-product of that active reading. Whenever I see something that looks a bit odd to me, I experiment a bit. Sometimes I hit a wall, and then understand better why the oddity was necessary in the first place (and then I sometimes leave a comment in the code to help the next reader who comes later). And some other times, the oddity wasn't actually necessary, and we can just remove it. |
| zkvm_proof.raw_pi[0] = vec![<GoldilocksExt2 as ff_ext::ExtensionField>::BaseField::ONE]; | ||
| zkvm_proof.raw_pi[1] = vec![<GoldilocksExt2 as ff_ext::ExtensionField>::BaseField::ONE]; | ||
|
|
||
| // capture panic message, if have | ||
| let default_hook = panic::take_hook(); |
There was a problem hiding this comment.
@lispc Here's an example where I had some (minor) trouble understanding what the original code tried to do. So I cleaned it up into its own helper function, in order to make the job of the next reader down the line a bit easier.
|
Ha, interesting we get some test failures! Seems like I didn't understand the original code well enough. Time to learn something. |
| @@ -35,6 +35,24 @@ struct Args { | |||
| max_steps: Option<usize>, | |||
| } | |||
|
|
|||
| fn with_panic_hook<F, R>(hook: Box<dyn Fn(&PanicHookInfo<'_>) + Sync + Send + 'static>, f: F) -> R | |||
There was a problem hiding this comment.
Might as well go to the root cause and change the assert to a returned error.
| } | ||
| let mut padded = self.padded(new_len, records); | ||
| padded.sort_by_key(|record| record.addr); | ||
| padded |
There was a problem hiding this comment.
This does extra work without being clearer when we just want the default content. If you really want to remove that if then it's more meaningful to split it into two functions.
There was a problem hiding this comment.
Oh, I didn't actually care that much about the if. It was more about converting from vector to iterator (then internally in sorted_by_key convert back to vector and sort then convert back to iterator) and then back to vector again. I'm not at all against letting the computer do some extra work, but here it didn't even make the code easier to read.
But while I was at it, I also simplified the structured by removing the if. The fewer branching code paths you have, the easier it is to achieve good test coverage of all the code paths. Rust's sorting is very efficient for already sorted data: it finishes in O(n). And we need O(n) anyway just to produce the data in the first place.
There was a problem hiding this comment.
Ho ok if that was about speed, I think that is zero-cost, it does the same thing.
There was a problem hiding this comment.
| @@ -209,7 +230,7 @@ fn main() { | |||
| MemFinalRecord { | |||
| addr: rec.addr, | |||
| value: vm.peek_memory(vma), | |||
| cycle: *final_access.get(&vma).unwrap_or(&0), | |||
| cycle: final_access.get(&vma).copied().unwrap_or_default(), | |||
There was a problem hiding this comment.
0 is the intended value. No reason to hide it.
There was a problem hiding this comment.
I'm fine either way here.
| Some(self.step(&emu)) | ||
| } | ||
| }) | ||
| from_fn(move || self.halted().not().then(|| self.step(&emu))) |
There was a problem hiding this comment.
Why not something based on take_while?
There was a problem hiding this comment.
That's exactly what I thought as well! But both halted and step want to borrow self, so the borrow checker didn't like that.
I was very tempted to take that as a suggestion that we should refactor. But that would have been way beyond the narrow scope of what I was trying to do here.
If you make a PR to that effect, I'm happy to review.
There was a problem hiding this comment.
Got it. I suspect I had tried that too back then 🤣
There was a problem hiding this comment.
Perhaps we should leave a comment to save the next curious person some head scratching.
There was a problem hiding this comment.
There would be a lot of comments if we’d start documenting hypothetical things that don’t work.
While reviewing commit 63ad6f9 / PR #368 I came up with a few clean ups and improvements. Nothing earth shattering, but might as well fix it.
Sorry, I didn't have time to review it, before it went into
master.