StackOutputs: get_stack_{item, word}() methods
#1155
Conversation
core/src/stack/outputs.rs
Outdated
| /// Returns the value located at the specified position on the stack | ||
| pub fn get_stack_item(&self, idx: usize) -> Option<Felt> { | ||
| self.stack.get(idx).map(|&felt| felt.into()) | ||
| } |
There was a problem hiding this comment.
I wonder if this function should be fallible. If we try and access an element with idx > self.stack.len() - 1 then we should return an error?
There was a problem hiding this comment.
I prefer to return an Option when there's only one error possible (here: out of bounds). If we returned OutputError for example, and a caller wanted to match on the error, then they would need to handle error cases that could never happen. Then, if we create a new error type with one variant, I consider that "type bloat" because it doesn't convey any more information than an Option would, but introduces a new name.
I believe this is a convention widely used in the standard library, such as with slice::get.
There was a problem hiding this comment.
I updated the comments to document what None means here
| pub fn get_stack_word(&self, idx: usize) -> Option<Word> { | ||
| let word_elements: Word = { | ||
| let word_elements: Vec<Felt> = range(idx, 4) | ||
| .map(|idx| self.get_stack_item(idx)) | ||
| // Elements need to be reversed, since a word `[a, b, c, d]` will be stored on the | ||
| // stack as `[d, c, b, a]` | ||
| .rev() | ||
| .collect::<Option<_>>()?; | ||
|
|
||
| word_elements.try_into().expect("a Word contains 4 elements") | ||
| }; | ||
|
|
||
| Some(word_elements) | ||
| } |
There was a problem hiding this comment.
Similar to above, I wonder if this function should be fallible in the case we try and access a word which is out of bounds?
There was a problem hiding this comment.
Above comment applies
| @@ -1,3 +1,7 @@ | |||
| use miden_crypto::Word; | |||
|
|
|||
There was a problem hiding this comment.
typically we try and avoid spacing between imports.
There was a problem hiding this comment.
This can be solved by the group_imports section of rustfmt.toml. I see that it's been commented out, and the reason seems to be that the CI complained that the features were on nightly only. This is solved by configuring the CI to use nightly rust for the cargo fmt command. I will open another PR for it
|
Also, let's add an entry to the changelog for this. |
|
Added changelog entry under "VM Internals" header |
* get_stack_item() * get_stack_word * adjust comment * adjust comments * fix comment * fix comments
* get_stack_item() * get_stack_word * adjust comment * adjust comments * fix comment * fix comments
Followup to discussions in #1146