feat: refactor instruction execution#11
Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors the VM execution engine to use explicit stack frames for function calls, updates the program builder API, and adds a helper on Variant for falsy checks.
- Adds
Variant::is_falseto standardize falsy evaluation. - Overhauls
Vmto store recursive call frames (StackFrame) and switch toRc<Function>. - Introduces an empty
optimize()method onProgramBuilderand shiftsbuild()to only return the program.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/variant.rs | Added is_false method to unify falsy semantics across variants. |
| src/runtime.rs | Replaced single-frame execution with a Vec<StackFrame>, updated entry-point resolution and error messages. |
| src/builder.rs | Renamed build() to optimize() (empty) and restored build(self) to return Program. |
| examples/math.rs | New standalone example demonstrating basic VM usage. |
| examples/fib.rs | Updated example to use .end_function() instead of .return_value(). |
Comments suppressed due to low confidence (2)
src/runtime.rs:458
- Typo in error message: change
an dictionarytoa dictionaryfor correct grammar.
_ => return runtime_error!("Expected an dictionary but got {:?}", table)
examples/fib.rs:27
- Using
.end_function()here likely omits pushing the computed Fibonacci value onto the operand stack. Replace with.return_value()to ensure the function returns the result.
.end_function()
| id: 1, | ||
| function: f.clone(), | ||
| pc: 0, | ||
| locals: vec![Variant::Null; f.local_count], |
There was a problem hiding this comment.
The initial stack frame ignores the provided parameters and fills all locals with Null, discarding function arguments. Preload the first parameters.len() entries of locals with the actual parameters before padding.
| locals: vec![Variant::Null; f.local_count], | |
| locals: { | |
| let mut locals = Vec::with_capacity(f.local_count); | |
| locals.extend(parameters.iter().cloned()); // Preload with parameters | |
| locals.resize(f.local_count, Variant::Null); // Pad with Null | |
| locals | |
| }, |
|
|
||
| // Resolve function references with function index | ||
| for function in &mut self.program.functions { | ||
| for instruction in &mut function.instructions { | ||
| if let Instruction::FunctionCall(CallTarget::Name(name)) = instruction { | ||
| if let Some(SymbolEntry::UserDefinedFunction { index }) = self.program.symbol_table.get(name) { | ||
| *instruction = Instruction::FunctionCall(CallTarget::Index(*index)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // for function in self.program.functions { | ||
| // for instruction in function.instructions { | ||
| // if let Instruction::FunctionCall(CallTarget::Name(name)) = instruction { | ||
| // if let Some(SymbolEntry::UserDefinedFunction { index }) = self.program.symbol_table.get(name) { | ||
| // *instruction = Instruction::FunctionCall(CallTarget::Index(*index)); | ||
| // } | ||
| // } | ||
| // } | ||
| // } | ||
|
|
There was a problem hiding this comment.
[nitpick] The optimize method contains only commented-out code. Remove dead comments or implement the intended optimization to keep the codebase clean and avoid confusion.
| } | ||
| } | ||
|
|
||
| pub fn is_false(&self) -> bool { |
There was a problem hiding this comment.
[nitpick] Public method is_false lacks a doc comment. Adding a brief description—especially noting that Null is considered false—will clarify expected behavior for downstream users.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the instruction execution and updates the API call for vm.run across tests and runtime code while also performing minor adjustments to test inputs and function naming. Key changes include:
- Changing the vm.run signature from one to two parameters.
- Adjusting test function names and input values.
- Refactoring stack and function call handling in the runtime module.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/variables.rs | Renaming and addition of test functions to cover both variable and arg retrieval. |
| tests/math.rs | Updating vm.run calls across various math tests. |
| tests/functions.rs | Updating vm.run calls in function tests. |
| tests/fib.rs | Changing test input value and updating vm.run call. |
| tests/dictionary.rs | Updating vm.run calls in dictionary tests. |
| tests/controlflow.rs | Modifying jump target and updating vm.run call. |
| tests/arrays.rs | Updating vm.run calls in array tests. |
| src/variant.rs | Minor adjustment (newline) with no behavioral changes. |
| src/runtime.rs | Major refactoring replacing frame usage with a stack-based approach. |
| src/builder.rs | Separating optimize and build functionality in the Program builder. |
| examples/* | Updating vm.run calls and logger initialization in example programs. |
| benches/* | Updating vm.run calls in benchmark files. |
Comments suppressed due to low confidence (2)
tests/variables.rs:4
- There is a renaming of the test function from 'test_get_variable' to 'test_get_arg', while later a separate 'test_get_variable' is added. Consider clarifying the naming so that each test’s purpose is clearly differentiated.
fn test_get_arg() {
tests/fib.rs:6
- The test input value is reduced from 10 to 5, but the surrounding comments or context (if any) may still imply the original value. Verify that this change is intentional and consistent with the expected Fibonacci computation.
let input = 5;
| pc = 0; | ||
| stack_base_pointer = stack.len() - next_function.arity; | ||
|
|
||
| // Extend the stack with the arguments |
There was a problem hiding this comment.
[nitpick] The refactored stack manipulation logic is more complex; adding inline comments to explain the intended invariants and the purpose of resizing could help future maintenance and debugging.
| // Extend the stack with the arguments | |
| // Extend the stack to accommodate the local variables of the new function. | |
| // Invariant: The stack must have enough space for all local variables, which are | |
| // initialized to `Variant::Null`. The size of the stack after resizing is determined | |
| // by adding the number of local variables (`current_function.local_count`) to the | |
| // base pointer of the current stack frame (`stack_base_pointer`). |
No description provided.