Correct extract_if sample equivalent.
#135734
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @the8472 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
Checking in on this. It would be nice if this could be fixed before |
| /// # let some_predicate = |x: &mut i32| { *x % 2 == 1 }; | ||
| /// # let mut vec = vec![0, 1, 2, 3, 4, 5, 6]; | ||
| /// # let range = 1..5; | ||
| /// let mut i = range.start; | ||
| /// while i < min(vec.len(), range.end) { | ||
| /// let mut end = range.end; | ||
| /// while i < min(vec.len(), end) { | ||
| /// if some_predicate(&mut vec[i]) { | ||
| /// let val = vec.remove(i); | ||
| /// end -= 1; | ||
| /// // your code here | ||
| /// } else { | ||
| /// i += 1; | ||
| /// } | ||
| /// } | ||
| /// | ||
| /// # assert_eq!(vec, vec![1, 4, 5]); | ||
| /// # assert_eq!(vec, vec![0, 2, 4, 5, 6]); |
There was a problem hiding this comment.
I don't think it needs to exactly match the example below but it should match extract_if, could you add a check against this? Something like:
# let mut vec = vec![...];
# let mut vec2 = vec.clone();
# let mut extracted = vec![];
// ...
// your code here
# extracted.push(val);
// ...
# let extracted2 = vec2.extract_if(range, some_predicate).collect::<Vec<_>>();
# assert_eq!(vec, vec2);
# assert_eq!(extracted, extracted2);| /// let mut end = range.end; | ||
| /// while i < min(vec.len(), end) { | ||
| /// if some_predicate(&mut vec[i]) { | ||
| /// let val = vec.remove(i); | ||
| /// end -= 1; |
There was a problem hiding this comment.
Isn't this a redundant op? end and vec.len() only ever change at the same time, so it should be possible to do something like while i < vec.len() - diff, where diff is vec.len() - range.end using the original vec.len().
|
Thanks for the PR! Agreed that this should be updated, but I think it could be simplified a bit and we should be able to do a better assertion. |
tgross35
added
S-waiting-on-author
Tracking issue: #43244
Original PR: #133265
The sample code marked as equivalent in the doc comment isn't currently equivalent. Given the same predicate and range, if your vector were
[1, 2, 3, 3, 3, 3, 3, 3, 4, 5, 6], then all of the 3s would be removed.iis only incremented when an element is dropped, butrange.endis unchanged, so the items shift down. I got very confused when reading the docs and trying to square this sample code with the explanation of how the function works.Fortunately, the real
extract_if()does not have this problem. I've added anendvariable to align the behavior. I've also taken the opportunity to simplify the predicate, which now just matches odd numbers, and to pad out the vec of numbers to line up the zero-indexed range with the integers in the vec.r? the8472