WIP: Session 6 functionality#5
Conversation
danielsilverstone-ct
left a comment
danielsilverstone-ct
left a comment
There was a problem hiding this comment.
Over-all I think you've done well. If you say that this passes everything you can think of to throw at it then I'm happy with the functionality.
I think there's work to be done with respect to deciding what things need to be pub vs. not; remember that anything you make public you are making part of your support contract for future development.
If you want to chat about things, please find me.
| fn next_instruction(&self) -> usize { | ||
| self.instruction_pointer + 1 | ||
| } | ||
|
|
There was a problem hiding this comment.
This function feels a little "extra" but I don't hate it.
| pub fn current_cell(&mut self) -> &mut T { | ||
| &mut self.cells[self.head] | ||
| } |
There was a problem hiding this comment.
I'm not clear why you need this function, using this can cause borrow problems in functions in your impl blocks, because to call it, everything inside the instance must be consistent and unborrowed - I imagine you're not having a problem right now, but in general I try and avoid this kind of function unless it's critical to your API. Do you need this to be public? If not, perhaps it need not exist.
If you're not quite sure why I'm saying this is a potential problem; then think more about what it means to be able to call a method on a struct in terms of the borrows. If you're still stuck, poke me IRL and I'll try to explain it another way.
| file.write_all(&buffer) | ||
| .and_then(|_| file.flush()) | ||
| .map_err(|e| VMError::IOError { | ||
| instruction: self.current_instruction().instruction(), | ||
| source: e, | ||
| }) | ||
| .and(Ok(self.next_instruction())) |
There was a problem hiding this comment.
This is a really nice literate handling of the write/flush/map_err process, well done.
| assert!(matches!( | ||
| self.current_instruction(), | ||
| DecoratedInstruction::OpenLoop { .. } | ||
| )); |
There was a problem hiding this comment.
Surely this function won't be called unless the current instruction is an openloop?
| .prog | ||
| .position_to_index(closer.line(), closer.character()) | ||
| + 1), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
The assert/matches above is also handled here since unreachable!() will panic if it's ever reached.
| closer, | ||
| } => Ok(self | ||
| .prog | ||
| .position_to_index(closer.line(), closer.character()) |
There was a problem hiding this comment.
This looks like a conversion which could have been precomputed and stored in the instruction stream.
While I wouldn't always advocate precomputing things, in this instance the use of this data will be frequent and on the critical path of most BF programs.
| assert!(matches!( | ||
| self.current_instruction(), | ||
| DecoratedInstruction::CloseLoop { .. } | ||
| )); |
There was a problem hiding this comment.
Same comments as above re: assert/matches and the unreachable below
| opener, | ||
| } => Ok(self | ||
| .prog() | ||
| .position_to_index(opener.line(), opener.character())), |
There was a problem hiding this comment.
And for this computation step
| self.instruction_pointer = self.interpret_current_instruction(input, output)? | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn interpret_current_instruction( | ||
| &mut self, | ||
| input: &mut impl Read, | ||
| output: &mut impl Write, | ||
| ) -> Result<usize, VMError> { | ||
| match self.current_instruction().instruction().instruction() { | ||
| RawInstruction::OpenLoop => self.open_loop(), | ||
| RawInstruction::CloseLoop => self.close_loop(), | ||
| RawInstruction::DecrementDataPointer => self.seek_left(), | ||
| RawInstruction::IncrementDataPointer => self.seek_right(), | ||
| RawInstruction::IncrementByte => self.increment_cell(), | ||
| RawInstruction::DecrementByte => self.decrement_cell(), | ||
| RawInstruction::GetByte => self.read_value(input), | ||
| RawInstruction::PutByte => self.write_value(output), | ||
| } |
There was a problem hiding this comment.
Personally I'd not have had a separate method for interpreting a single instruction, though this isn't entirely bad.
| .then(instruction.character().cmp(&character)) | ||
| }) | ||
| .unwrap() | ||
| // >:| I can't just search for the DecoratedInstruction because I don't store it |
There was a problem hiding this comment.
Why not store an index -> index map instead (perhaps as a Vec<usize>)?
3 participants
No description provided.