override VecDeque::try_rfold, also update iterator
#58064
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
r? @scottmcm |
scottmcm
reviewed
Feb 2, 2019
scottmcm
reviewed
Feb 2, 2019
scottmcm
added
S-waiting-on-author
Centril
reviewed
Feb 2, 2019
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
7bebb77
to
b026114
Compare
|
This should now work. I've cross-checked that it behaves like the old implemenation in all tests I wrote. |
|
I think there is a difference to the old version if I note that we make no guarantees for the panic case, so technically the current version should be correct. |
|
The idea is to have a This has the benefit of potentially needing less arithmetic and being panic-safe, at the cost of more unsafe code and bookkeeping. It should be safe to alias, because the original mutable borrow of the iterator has gone out of scope when the guard drops, right? |
|
I don't know the answer to panic-safety; opened #58170 to ask libs. |
tesuji
reviewed
Feb 5, 2019
| let res = front_iter.try_fold(init, &mut f); | ||
| // the slice's `try_fold` leaves the iterator where we want our `tail` to be | ||
| self.tail = unsafe { front_iter.as_slice().as_ptr() | ||
| .offset_from(self.ring.as_ptr()) as usize }; |
There was a problem hiding this comment.
I think this should be format like this (by default rustfmt config):
self.tail = unsafe {
front_iter
.as_slice()
.as_ptr()
.offset_from(self.ring.as_ptr()) as usize
};
tesuji
reviewed
Feb 5, 2019
| // same here | ||
| self.tail = unsafe { back_iter.as_slice().as_ptr() | ||
| .offset_from(self.ring.as_ptr()) as usize }; | ||
| res |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tesuji
reviewed
Feb 5, 2019
| let res = back_iter.try_rfold(init, &mut f); | ||
| // the slice's `try_rfold` leaves the iterator where we want our `head` to be | ||
| self.head = unsafe { back_iter.as_slice().as_ptr() | ||
| .offset_from(self.ring.as_ptr()) as usize }; |
tesuji
reviewed
Feb 5, 2019
| let res = front_iter.try_rfold(res?, f); | ||
| // same here | ||
| self.head = unsafe { front_iter.as_slice().as_ptr() | ||
| .offset_from(self.ring.as_ptr()) as usize }; |
tesuji
reviewed
Feb 5, 2019
llogiq
force-pushed
the
vec-deque-try-rfold
branch
2 times, most recently
from
a184f53
to
eade0b7
Compare
|
rustfmt'd and rebased |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Oops, it appears I made an error during the merge. |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
eade0b7
to
98753ff
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
666c3e9
to
c2d7c8d
Compare
|
I think I adressed all comments now. r? @scottmcm |
scottmcm
requested changes
Feb 11, 2019
| self.tail = self.ring.len() - front_iter.len(); | ||
| let mut back_iter = back.iter(); | ||
| let res = back_iter.try_rfold(res?, f); | ||
| self.tail = back.len() - back_iter.len(); |
There was a problem hiding this comment.
I'm concerned here again. Suppose we did
let mut v = VecDeque::with_capacity(7);
v.push_back(1);
v.push_back(1);
v.push_back(1);
v.pop_front();
v.pop_front();Then the backing store is
_ _ 1 _ _ _ _ _
head = 2
tail = 3so (front, back) == ([1], []), and iterating will result in setting tail = 0 - 0, leaving an iterator that thinks it contains 6 elements, and safe code would then be exposed to uninitialized values. (Not to mention that the iter implements FusedIterator, so it's not supposed to return non-None after returning None once.)
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
c2d7c8d
to
8d61424
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
8d61424
to
d2e2371
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
d2e2371
to
cc3aa33
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
cc3aa33
to
c80e471
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
c80e471
to
2148cd4
Compare
|
I inlined the ring_slices and made the code more obvious. It seems to pass all tests. |
scottmcm
requested changes
Feb 18, 2019
| let (front, back) = self.ring.split_at(self.tail); | ||
| let mut back_iter = back.iter(); | ||
| let res = back_iter.try_fold(init, &mut f); | ||
| self.tail = self.ring.len() - back_iter.len(); |
There was a problem hiding this comment.
next will unchecked read from tail, so I wanted to see if I could convince myself that a back-iter of zero length wouldn't ever be seen here, but I don't think that's guaranteed. Take this test:
use std::collections::VecDeque;
let mut v = VecDeque::with_capacity(8);
v.push_back(7);
v.push_back(8);
v.push_back(9);
v.push_front(2);
v.push_front(1);
assert_eq!(v.as_slices(), (&[1, 2][..], &[7, 8, 9][..]));
let mut it = v.iter();
it.find(|&&x| x == 2);
assert_eq!(it.next(), Some(&7));That sets up the storage as
7 8 9 _ _ _ 1 2
h t
then the find call returns an break from the first try_fold, but after consuming all the items. So tail gets set to ring.len(), and then the function returns early from the ? on line 2191. Then the call to next reads uninitialized memory.
Does that sound right, or is there something I missed? (I didn't run this iteration locally.)
There was a problem hiding this comment.
You're right. I need a wrapping operation on the fold (though not on the rfold, because in that case the iterator is already placed at the correct element, right?).
There was a problem hiding this comment.
I think so, since neither slice can be as big as the underlying capacity.
This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code. This uses unsafe code, so some thorough review would be appreciated.
llogiq
force-pushed
the
vec-deque-try-rfold
branch
from
2148cd4
to
64c915e
Compare
scottmcm
approved these changes
Feb 22, 2019
|
@bors r+ |
|
📌 Commit 64c915e has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Feb 22, 2019
override `VecDeque::try_rfold`, also update iterator This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code. This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung
Centril
added a commit
to Centril/rust
that referenced
this pull request
Feb 22, 2019
override `VecDeque::try_rfold`, also update iterator This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code. This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung
bors
added a commit
that referenced
this pull request
Feb 22, 2019
Rollup of 17 pull requests Successful merges: - #57656 (Deprecate the unstable Vec::resize_default) - #58059 (deprecate before_exec in favor of unsafe pre_exec) - #58064 (override `VecDeque::try_rfold`, also update iterator) - #58198 (Suggest removing parentheses surrounding lifetimes) - #58431 (fix overlapping references in BTree) - #58555 (Add a note about 2018e if someone uses `try {` in 2015e) - #58588 (remove a bit of dead code) - #58589 (cleanup macro after 2018 transition) - #58591 (Dedup a rustdoc diagnostic construction) - #58600 (fix small documentation typo) - #58601 (Search for target_triple.json only if builtin target not found) - #58606 (Docs: put Future trait into spotlight) - #58607 (Fixes #58586: Make E0505 erronous example fail for the 2018 edition) - #58615 (miri: explain why we use static alignment in ref-to-place conversion) - #58620 (introduce benchmarks of BTreeSet.intersection) - #58621 (Update miri links) - #58632 (Make std feature list sorted) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This keeps the slice based iteration and updates the iterator state after each slice. It also uses a loop to reduce the amount of code.
This uses unsafe code, so some thorough review would be appreciated. Cc @RalfJung