Implement Range for OpStack #338
Conversation
|
Thanks for taking this on! Unfortunately, there is a logic error in the implementation. The documentation of field /// The underlying, actual stack. When manually accessing, be aware of reversed indexing:
/// while `op_stack[0]` is the top of the stack, `op_stack.stack[0]` is the lowest element in
/// the stack.
Consequently, accessing |
There was a problem hiding this comment.
This file is only generated due to a bug in proptest. It has purposefully not been commited.
| @@ -4,6 +4,7 @@ use std::fmt::Result as FmtResult; | |||
| use std::num::TryFromIntError; | |||
| use std::ops::Index; | |||
| use std::ops::IndexMut; | |||
| use std::ops::{Range, RangeInclusive}; | |||
There was a problem hiding this comment.
In this repository, we use an import granularity of item in order to trivialize git merging of imports.
| if range.end <= self.stack.len() && range.start < range.end { | ||
| &self.stack[range] | ||
| } else { | ||
| panic!("a range is out of bounds") |
There was a problem hiding this comment.
I think's it's cleaner to just use the underlying panics, should there be any:
- By not implementing these checks yourself, you cannot introduce logic bugs in them. Concretely, the range check in the
impl Index<RangeInclusive<usize>> for OpStacklooks suspiciously similar to the one inimpl Index<Range<usize>> for OpStack. - The panic's message will correctly reflect the kind of access violation. For example, if the range is invalid, the current implementation will display an error regarding range bounds, whereas rust's panic message reads “slice index starts at
xbut ends aty.” - By dropping the manual bounds-check here people will see the accustomed message “index out of bounds: the len is
xbut the index isy.” - The stacktrace will point to the relevant method in either case.
Closes #322