feat: Builder pattern for VirtualMachine #820
Conversation
src/vm/vm_core.rs
Outdated
| } | ||
|
|
||
| pub fn segments(mut self, segments: MemorySegmentManager) -> VirtualMachineBuilder { | ||
| // Set the name on the builder itself, and return the builder by value. |
There was a problem hiding this comment.
You copy pasted this comment on every setter method.
Yon can remove it every time
src/vm/vm_core.rs
Outdated
| // If we can get away with not consuming the Builder here, that is an | ||
| // advantage. It means we can use the FooBuilder as a template for constructing | ||
| // many Foos. |
src/vm/vm_core.rs
Outdated
| // Create a Foo from the FooBuilder, applying all settings in FooBuilder | ||
| // to Foo. |
src/vm/vm_core.rs
Outdated
| } | ||
|
|
||
| impl VirtualMachineBuilder { | ||
| pub fn new(trace_enabled: bool) -> VirtualMachineBuilder { |
There was a problem hiding this comment.
Why is trace enable treated differently than the other fields?
Couldn't it also have it setter method?
There was a problem hiding this comment.
It also has one but I just reproduced VirtualMachine constructor behavior. Don't know if it's the right way of doing it or if it should be created empty.
There was a problem hiding this comment.
You can ignore the current VirtualMachine::new and go with your own. By default it will be None, and if the function with_trace (or smthing else) is called, it will internally be set to Some(Vec::<TraceEntry>::new())
zarboq
force-pushed
the
virtual_machine_builder
branch
from
d161a24 to
1ff9260
Compare
Codecov Report
@@ Coverage Diff @@
## main #820 +/- ##
==========================================
+ Coverage 96.82% 96.84% +0.01%
==========================================
Files 69 69
Lines 28835 29017 +182
==========================================
+ Hits 27920 28102 +182
Misses 915 915
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
also it's breaking the CI |
zarboq
force-pushed
the
virtual_machine_builder
branch
3 times, most recently
from
356bd99 to
34ee83d
Compare
src/vm/vm_core.rs
Outdated
| impl Default for VirtualMachineBuilder { | ||
| fn default() -> Self { | ||
| Self::new() | ||
| } |
There was a problem hiding this comment.
Perso si j'ai default et new qui font la même chose je préfère autant n'avoir que default.
C'est pas un red flag, mais c'est plus concis, respectueux des principe DRY
src/vm/vm_core.rs
Outdated
|
|
||
| #[test] | ||
| fn builder_test() { | ||
| let run_context = RunContext { |
There was a problem hiding this comment.
inline RunContext { ... } in the VitualMachine { ... } instanciation
src/vm/vm_core.rs
Outdated
| fp: 0, | ||
| }; | ||
|
|
||
| let virtual_machine = VirtualMachine { |
There was a problem hiding this comment.
call it expected_virtual_machine
zarboq
force-pushed
the
virtual_machine_builder
branch
from
34ee83d to
dc87676
Compare
| .run_finished(true) | ||
| .current_step(12) |
There was a problem hiding this comment.
Add a call to each one of your setters function here
zarboq
force-pushed
the
virtual_machine_builder
branch
2 times, most recently
from
8f8b003 to
6feafeb
Compare
src/vm/vm_core.rs
Outdated
| let _virtual_machine_from_builder: VirtualMachine = VirtualMachineBuilder::default() | ||
| .hooks(crate::vm::hooks::Hooks::new( | ||
| None, | ||
| Some(Arc::new(pre_step_hook)), |
There was a problem hiding this comment.
Some(std::sync::Arc::new(pre_step_hook)), will spare you the import of line 4200
src/vm/vm_core.rs
Outdated
| ap: Relocatable::from((0, 1)), | ||
| fp: Relocatable::from((0, 1)), | ||
| }])) | ||
| .build(); |
There was a problem hiding this comment.
About the test of the hook.
Don't call .build() immediately. Assign the builder to a variable instead.
Then behind #[cfg(feature = "hooks")] call hooks on this variable.
Then call build() and store the vm_from_builder as you did.
Then do all your assert, then behind #[cfg(feature = "hooks")] add one more assert for the hooks
zarboq
force-pushed
the
virtual_machine_builder
branch
from
6feafeb to
04b9321
Compare
zarboq
requested review from
igaray,
Oppen,
fmoletta,
entropidelic,
HermanObst,
juanbono,
Jrigada,
pefontana and
Juan-M-V
as code owners
|
@Oppen would you like to be the second approver? |
Builder pattern for VirtualMachine
Description
Added a builder pattern for VirtualMachine (VirtualMachineBuilder) to allow creating VirtualMachine with customized fields.
Linked to issue #819
Checklist