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

Remove Borrow bound from SliceExt::binary_search #43989

Merged
merged 1 commit into from
Sep 17, 2017

Conversation

circuitfox
Copy link
Contributor

#37761 added a Borrow bound to binary_search and binary_search_by_key in core::SliceExt, but did not add it to the methods in std::slice. #41590 attempted to add this bound to std::slice but was not merged due to breakage. This PR removes the bound in core::SliceExt, so that these methods will have the same signature in core and std.

Fixes #41561

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

@bors try -- gathering artifacts for cargobomb

@bors
Copy link
Contributor

bors commented Aug 20, 2017

⌛ Trying commit ac15987 with merge a1c522c...

bors added a commit that referenced this pull request Aug 20, 2017
Remove Borrow bound from SliceExt::binary_search

#37761 added a Borrow bound to `binary_search` and `binary_search_by_key` in `core::SliceExt`, but did not add it to the methods in `std::slice`. #41590 attempted to add this bound to `std::slice` but was not merged due to breakage. This PR removes the bound in `core::SliceExt`, so that these methods will have the same signature in `core` and `std`.

Fixes #41561
@bors
Copy link
Contributor

bors commented Aug 20, 2017

☀️ Test successful - status-travis
State: approved= try=True

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2017
@carols10cents
Copy link
Member

review ping @aturon! pinging you on IRC as well!

@arielb1 arielb1 added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 29, 2017

I think this is waiting for a cargobomb? ping @aidanhs

@arielb1
Copy link
Contributor

arielb1 commented Aug 29, 2017

Umm this seems in limbo. I think @Mark-Simulacrum is on leave? And he wants a crater run?

@arielb1 arielb1 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 29, 2017
@Mark-Simulacrum
Copy link
Member

I'm back now, though still catching up with backlog. I try-ed this so that we can get statistics from cargobomb on how many crates this breaks. In #41590 we tried to generalize it in std, but that was found too problematic (inference breakage, I believe) so this attempts to de-generalize core. We need a cargobomb run to determine whether that is possible.

@aidanhs
Copy link
Member

aidanhs commented Aug 30, 2017

Typically I wait for a review and a ping from a member of the relevant team to rust-lang/infra with an explicit request so we don't cargobomb needlessly.

That said, we have some free capacity right now so I'll give it a go. Could you rebase in the meantime @circuitfox?

@circuitfox circuitfox force-pushed the sliceext-binary-search-sig branch from ac15987 to e15a07a Compare August 30, 2017 17:08
@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 5, 2017
@aidanhs
Copy link
Member

aidanhs commented Sep 5, 2017

Cargobomb results: http://cargobomb-reports.s3.amazonaws.com/pr-43989/index.html

Blacklist (spurious failures etc) at https://github.com/rust-lang-nursery/cargobomb/blob/master/blacklist.md. If you see any spurious failures not on the list, please make a PR against that file.

@carols10cents
Copy link
Member

ping @aturon there are cargobomb results for you, so I think this is ready for review!

@alexcrichton
Copy link
Member

Looks like all the failure there are spurious, so...

@bors: r+

Thanks @circuitfox!

@bors
Copy link
Contributor

bors commented Sep 14, 2017

📌 Commit e15a07a has been approved by alexcrichton

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017
…ig, r=alexcrichton

Remove Borrow bound from SliceExt::binary_search

rust-lang#37761 added a Borrow bound to `binary_search` and `binary_search_by_key` in `core::SliceExt`, but did not add it to the methods in `std::slice`. rust-lang#41590 attempted to add this bound to `std::slice` but was not merged due to breakage. This PR removes the bound in `core::SliceExt`, so that these methods will have the same signature in `core` and `std`.

Fixes rust-lang#41561
@bors
Copy link
Contributor

bors commented Sep 16, 2017

⌛ Testing commit e15a07a with merge b492405...

bors added a commit that referenced this pull request Sep 16, 2017
…richton

Remove Borrow bound from SliceExt::binary_search

#37761 added a Borrow bound to `binary_search` and `binary_search_by_key` in `core::SliceExt`, but did not add it to the methods in `std::slice`. #41590 attempted to add this bound to `std::slice` but was not merged due to breakage. This PR removes the bound in `core::SliceExt`, so that these methods will have the same signature in `core` and `std`.

Fixes #41561
@bors
Copy link
Contributor

bors commented Sep 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing b492405 to master...

@bors bors merged commit e15a07a into rust-lang:master Sep 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants