Implement parsing of pushw and env io operations #40
Conversation
…immediate values are provided
There was a problem hiding this comment.
Looks great! Thank you! I left a couple of notes for the future in-line. Regarding checking for exact errors: I actually don't know what the best practice on this is - but yes, I agree, ideally we should be checking against exact errors. Could you create an issue for investigating this?
| /// This is a helper function that validates the length of the assembly io instruction and returns | ||
| /// an error if params are missing or there are extras. | ||
| /// | ||
| /// The minimum and maximum number of valid instruction parts for the op are provided by the | ||
| /// caller. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// This function will return an AssemblyError if the assembly op has too many or too few parts. | ||
| fn validate_op_len(op: &Token, min_parts: usize, max_parts: usize) -> Result<(), AssemblyError> { | ||
| match op.num_parts() { | ||
| too_few if too_few < min_parts => Err(AssemblyError::missing_param(op)), | ||
| too_many if too_many > max_parts => Err(AssemblyError::extra_param(op)), | ||
| _ => Ok(()), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Not for this PR, but I wonder if we should move this out to be used in other modules as well.
There was a problem hiding this comment.
Yes, I think we should. Otherwise we're either duplicating code or we're under-validating. Pulling this logic out & calling the function instead of using match should make all of the op parsing implementations more readable.
| fn pushw() { | ||
| // pushes a word of 4 immediate values in decimal or hexadecimal onto the stack | ||
| let mut span_ops: Vec<Operation> = Vec::new(); | ||
| let op = Token::new("pushw.1.23.0x1C8.0", 0); |
There was a problem hiding this comment.
Not for this PR: now that I'm seeing how this actually looks, I wonder if we should change the format of pushw instruction to always take a single hexadecimal value of 32 bytes. Or maybe making this as an additional allowed format. So, for example, possible formats could be:
pushw.1.2.3.4pushw.0x13244...(32 bytes)
Let's create an issue to think this through later on.
There was a problem hiding this comment.
I opened an issue here: #44
|
Errors issue with the result of my investigation is here: #43 |
I added a few tests to check error cases and couldn't assert_eq the exact errors. I guess this is because the PartialEq trait isn't implemented for AssemblyError, but maybe there's another way around this that I'm not aware of yet..? For the moment, I just asserted that the result gave an error without checking to see which one. Let me know if you have any thoughts on this.