Use strongly typed wrapped indices in VecDeque#152112
Use strongly typed wrapped indices in VecDeque#152112Kobzol wants to merge 4 commits intorust-lang:mainfrom
VecDeque#152112Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
38ead54 to
5aed3d9
Compare
| pub(super) struct WrappedIndex(usize); | ||
|
|
||
| impl WrappedIndex { | ||
| /// Safety invariant: the newly constructed index must be in-bounds for the VecDeque |
There was a problem hiding this comment.
I don't think this safety invariant is particularly useful, as all functions that act on the buffer still need to require that the WrappedIndex is specific to that particular VecDeque with a given capacity.
There was a problem hiding this comment.
Yeah, it's a relatively weak invariant. That being said, I still thought it would be a good idea to provide a "SLOW DOWN" sign when someone wants to conjure the index out of the blue, which is why I marked the function unsafe.
There was a problem hiding this comment.
I'm just not a big fan of redundant unsafe (this can neither cause UB itself nor be used in safety proofs). Isn't the WrappedIndex::new call enough of a stop-sign?
There was a problem hiding this comment.
Okay, fair enough. Maybe we could just name it something like from_arbitrary_number? 😆 Should I also remove unsafe from add and sub?
There was a problem hiding this comment.
Yeah, that sounds good. add and sub can stay unsafe, that would allow changing the adds and subs to unchanged in the future (if that turns out to be profitable).
| } | ||
|
|
||
| #[inline(always)] | ||
| pub(super) fn abs_diff(self, other: Self) -> Self { |
There was a problem hiding this comment.
The return type should be usize, right? It isn't a buffer index but rather a length.
There was a problem hiding this comment.
Right, the type represents something like "value in-bounds". My thought process was that if we have two in-bounds values, and we do an absolute diff of them, the result also has to be in-bounds (w.r.t. the same VecDeque). But we do treat it as usize everywhere the function is currently used, so.. I'll change it to usize.
|
I did name if |
|
I do slightly prefer the Wrapped name, I think that the buffer part is implied by the word "index". The main idea is to express that the index was wrapped to not go over bounds, which is how it is created through most means. @rustbot ready |
This comment has been minimized.
This comment has been minimized.
72a5f8f to
841763e
Compare
|
This probably needs a rebase atop #152258. Sorry for taking so long, staring at so many lines makes me dizzy, I've not been in the right mind to do it... |
841763e to
9c52ec0
Compare
This comment has been minimized.
This comment has been minimized.
|
Huh, the rebase went cleanly without any conflict, surprising.. 😆 No problem, I know from experience how draining it can be to review large diffs. Thank you very much for taking a look! |
| let tail_len = tail; | ||
| let head_len = cap - head.as_index(); | ||
|
|
||
| // Safety: tail <= head_len <= len <= capacity |
There was a problem hiding this comment.
tail <= head_len isn't correct (tail <= head is, though).
There was a problem hiding this comment.
Good catch. And also the <= len comparison doesn't hold either. Changed it to tail <= head < capacity.
| pub(super) struct WrappedIndex(usize); | ||
|
|
||
| impl WrappedIndex { | ||
| /// Safety invariant: the newly constructed index must be in-bounds for the VecDeque |
There was a problem hiding this comment.
Yeah, that sounds good. add and sub can stay unsafe, that would allow changing the adds and subs to unchanged in the future (if that turns out to be profitable).
9c52ec0 to
31abab1
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
31abab1 to
8044982
Compare
This comment has been minimized.
This comment has been minimized.
8044982 to
5ee2f79
Compare
This comment has been minimized.
This comment has been minimized.
5ee2f79 to
aa9f27e
Compare
View all comments
This is far from perfect, but it would have prevented #151769.
r? @joboet