Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for VecDeque binary search functions #78021

Closed
5 tasks done
vojtechkral opened this issue Oct 16, 2020 · 10 comments
Closed
5 tasks done

Tracking Issue for VecDeque binary search functions #78021

vojtechkral opened this issue Oct 16, 2020 · 10 comments
Labels
A-collections Area: std::collections. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@vojtechkral
Copy link
Contributor

vojtechkral commented Oct 16, 2020

Feature gate: #![feature(vecdeque_binary_search)]

This is a tracking issue for VecDeque::binary_search{,_by,_by_key}(), formerly RFC rust-lang/rfcs#2997

Public API

// alloc::collections::vec_deque

impl<T> VecDeque<T> {
    pub fn binary_search(&self, x: &T) -> Result<usize, usize>
    where
        T: Ord;

    pub fn binary_search_by<'a, F>(&'a self, f: F) -> Result<usize, usize>
    where
        F: FnMut(&'a T) -> Ordering;

    pub fn binary_search_by_key<'a, B, F>(&'a self, b: &B, f: F) -> Result<usize, usize>
    where
        F: FnMut(&'a T) -> B,
        B: Ord;

    pub fn partition_point<P>(&self, mut pred: P) -> usize
    where
        P: FnMut(&T) -> bool;
}

Steps / History

Unresolved Questions

@vojtechkral vojtechkral added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Oct 16, 2020
@vojtechkral vojtechkral changed the title Tracking Issue for XXX Tracking Issue for VecDeque binary search functions Oct 16, 2020
@JohnTitor JohnTitor added B-unstable Blocker: Implemented in the nightly compiler and unstable. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 16, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Nov 6, 2020
@KodrAus KodrAus added the A-collections Area: std::collections. label Jan 6, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Mar 23, 2021

We recently changed slice::binary_search_by to stop directly at Ordering::Equal, and added slice::partition_point for a binary search that only considers less and greater than. Maybe we should do the same for VecDeque.

@vojtechkral
Copy link
Contributor Author

@m-ou-se Okay, I can do that...

@vojtechkral
Copy link
Contributor Author

vojtechkral commented Mar 24, 2021

@m-ou-se I added the quick return for Ordering::Equal (locally in my branch so far), but I'm not sure about partition_point(). Do you think that's useful for a VecDeque too?

@m-ou-se
Copy link
Member

m-ou-se commented Mar 27, 2021

I added the quick return for Ordering::Equal (locally in my branch so far)

Feel free to send that as a PR and assign me with r? @m-ou-se.

I'm not sure about partition_point(). Do you think that's useful for a VecDeque too?

I don't have a very strong opinion on that. I think it might be useful, but I'd be fine with starting the FCP for just the three VecDeque::binary_search* functions.

@vojtechkral
Copy link
Contributor Author

vojtechkral commented Apr 12, 2021

Sorry about the radio silence, I've added partition_point() as well, will create the PR tomorrow...
PR: #84145

vojtechkral added a commit to vojtechkral/rust that referenced this issue Apr 15, 2021
vojtechkral added a commit to vojtechkral/rust that referenced this issue Apr 15, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 16, 2021
… r=m-ou-se

Address comments for vecdeque_binary_search rust-lang#78021
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 16, 2021
Rollup of 4 pull requests

Successful merges:

 - rust-lang#83337 (rustdoc: Hide item contents, not items)
 - rust-lang#83944 (Fix a couple resolve bugs from binder refactor)
 - rust-lang#84145 (Address comments for vecdeque_binary_search rust-lang#78021)
 - rust-lang#84172 (Compiler error messages: reduce assertiveness of message E0384)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@m-ou-se
Copy link
Member

m-ou-se commented May 17, 2021

@rfcbot merge

@rfcbot
Copy link

rfcbot commented May 17, 2021

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 17, 2021
@rfcbot
Copy link

rfcbot commented May 21, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 21, 2021
@rfcbot
Copy link

rfcbot commented May 31, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 31, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 3, 2021
…=m-ou-se

Stabilize `vecdeque_binary_search`

This PR stabilizes the feature `vecdeque_binary_search` as tracked in rust-lang#78021.

The tracking issue has not received any comments for the past 5 months, and concerns have been raised neither in the RFC rust-lang/rfcs#2997 nor in the tracking issue rust-lang#78021.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 3, 2021
…=m-ou-se

Stabilize `vecdeque_binary_search`

This PR stabilizes the feature `vecdeque_binary_search` as tracked in rust-lang#78021.

The tracking issue has not received any comments for the past 5 months, and concerns have been raised neither in the RFC rust-lang/rfcs#2997 nor in the tracking issue rust-lang#78021.
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 10, 2021
@JohnTitor
Copy link
Member

It has been stabilized (#83362), closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants