Skip to content

Add a range argument to vec.extract_if#133265

Merged
bors merged 4 commits into
rust-lang:masterfrom
the8472:extract-if-ranges
Dec 18, 2024
Merged

Add a range argument to vec.extract_if#133265
bors merged 4 commits into
rust-lang:masterfrom
the8472:extract-if-ranges

Conversation

@the8472

@the8472 the8472 commented Nov 20, 2024

Copy link
Copy Markdown
Member

tracking issue: #43244

This adds the range argument requested in #43244 (comment)

@rustbot

rustbot commented Nov 20, 2024

Copy link
Copy Markdown
Collaborator

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 20, 2024
@rust-log-analyzer

This comment has been minimized.

/// ```
/// #![feature(extract_if)]
/// let mut items = vec![0, 0, 0, 0, 0, 0, 0, 1, 2, 1, 2, 1, 2];
/// let ones = items.extract_if(7.., |x| *x == 1).collect::<Vec<_>>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change one of the first few 0s to 1 to indicate they don't get extracted (out of range)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I wanted to show the "I have a prefix that I know doesn't contain those things, so I can skip them" use.

@rust-log-analyzer

This comment has been minimized.

Comment thread library/alloc/src/vec/extract_if.rs
Comment thread library/alloc/src/vec/extract_if.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@cuviper cuviper left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The FCP finished -- do you want to go ahead and add the stabilization too?

Comment thread library/alloc/tests/vec.rs Outdated
@tgross35

tgross35 commented Dec 6, 2024

Copy link
Copy Markdown
Contributor

r? @cuviper (sorry, I didn't realize this was assigned to me)

The FCP finished -- do you want to go ahead and add the stabilization too?

Since this is a pretty significant API and implementation change, maybe it would be good to hold off for a couple of weeks? In case anyone using this API unstably has feedback after the change (I doubt it, but doesn't hurt).

@rustbot rustbot assigned cuviper and unassigned tgross35 Dec 6, 2024
@the8472

the8472 commented Dec 6, 2024

Copy link
Copy Markdown
Member Author

Changing and stabilizing in the same PR doesn't seem ideal to me either, especially when the change happened parallel to the FCP. I think it should bake a few days at least.

@cuviper

cuviper commented Dec 6, 2024

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Dec 6, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 414acda has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2024
@bors

bors commented Dec 16, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 44790c4 has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2024
@bors bors merged commit 938742e into rust-lang:master Dec 18, 2024
@rustbot rustbot added this to the 1.85.0 milestone Dec 18, 2024

@nk9 nk9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this is the right place to raise this question about the sample code in this PR.

/// ```
/// # use std::cmp::min;
/// # let some_predicate = |x: &mut i32| { *x == 2 || *x == 3 || *x == 6 };
/// # let mut vec = vec![1, 2, 3, 4, 5, 6];

@nk9 nk9 Jan 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the late comment, but I wanted to point out that this sample code doesn't behave the same way as the real extract_if(). 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. i is only incremented when an element is dropped, but range.end is 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. Proposed change to the sample code:

    let some_predicate = |x: &mut i32| { *x == 2 || *x == 3 || *x == 5 };
    let mut vec = vec![0, 1, 2, 3, 4, 5, 6];
    let range = 1..5;
    let mut i = range.start;
    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![0, 1, 4, 5, 6]);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to propose a documentation improvement you can just open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants