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

relaunch RangeBounds #44518

Closed
wants to merge 4 commits into from
Closed

relaunch RangeBounds #44518

wants to merge 4 commits into from

Conversation

liigo
Copy link
Contributor

@liigo liigo commented Sep 12, 2017

based on PR #43033 (by @clarcharr ) which is out of time for a bit long time

r? @sfackler or @alexcrichton

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 12, 2017
clarfonthey and others added 4 commits September 13, 2017 11:14
`Bound` is exported as `std::ops::Bound` and `alloc::Bound`,
cann't use an uniform relative path to link `BTreeMap` from here.
@sfackler
Copy link
Member

Thanks for updating this!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 15, 2017

📌 Commit 723b069 has been approved by sfackler

is_sync_send!(BTreeMap::<usize, usize>::new(), range((Included(&0), Included(&9))));
is_sync_send!(BTreeMap::<usize, usize>::new(), range_mut((Included(&0), Included(&9))));
is_sync_send!(BTreeMap::<usize, usize>::new(), range((Included(0), Included(9))));
is_sync_send!(BTreeMap::<usize, usize>::new(), range_mut((Included(0), Included(9))));
Copy link
Member

Choose a reason for hiding this comment

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

Was this change required to get this compiling? If so this may be a breaking change?

pub fn range<T: ?Sized, R>(&self, range: R) -> Range<K, V>
where T: Ord, K: Borrow<T>, R: RangeArgument<T>
pub fn range<T, R>(&self, range: R) -> Range<K, V>
where T: Ord, K: Borrow<T>, R: Into<RangeBounds<T>>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be RangeBounds<&T>?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I actually recall this being one of the things I struggled with making the original PR. This prevents things like "a".to_string()..="z".to_string(). Because Borrow works differently from AsRef, this prevents both String ranges and str ranges from working properly.

There may be a way to make it work but I was really stumped in the original PR and got busy and didn't pick it back up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm stumped too.

@sfackler
Copy link
Member

@bors r-

@bors
Copy link
Contributor

bors commented Sep 18, 2017

☔ The latest upstream changes (presumably #44678) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

ping @liigo, just want to make sure this doesn't fall off your radar!

@alexcrichton
Copy link
Member

Ok I'm going to close this due to inactivity, but feel free to resubmit @liigo!

@GuillaumeGomez
Copy link
Member

I'd really like to get this stabilized so if @liigo isn't available to di it, I'll gladly do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants