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

Implementation of plan in issue #27787 for btree_range #38610

Merged
merged 19 commits into from
Jan 15, 2017

Conversation

djzin
Copy link
Contributor

@djzin djzin commented Dec 26, 2016

Still some ergonomics to be worked on, the ::<str,_> is particularly unsightly

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

/// println!("{}: {}", key, value);
/// }
/// assert_eq!(Some((&5, &"b")), map.range(Included(&4), Unbounded).next());
/// assert_eq!(Some((&5, &"b")), map.range((Included(&4), Unbounded)).next());
Copy link
Member

Choose a reason for hiding this comment

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

This can be shortened to &4...

@@ -764,7 +764,7 @@ impl<K: Ord, V> BTreeMap<K, V> {
/// let mut map: BTreeMap<&str, i32> = ["Alice", "Bob", "Carol", "Cheryl"].iter()
/// .map(|&s| (s, 0))
/// .collect();
/// for (_, balance) in map.range_mut(Included("B"), Excluded("Cheryl")) {
/// for (_, balance) in map.range_mut::<str, _>((Included("B"), Excluded("Cheryl"))) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit weird that this would be required now?

The argument can be shortened to "B".."Cheryl".

/// println!("{}", elem);
/// }
/// assert_eq!(Some(&5), set.range(Included(&4), Unbounded).next());
/// assert_eq!(Some(&5), set.range((Included(&4), Unbounded)).next());
Copy link
Member

Choose a reason for hiding this comment

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

&4..

@sfackler
Copy link
Member

Woo!

@sfackler sfackler closed this Dec 26, 2016
@sfackler sfackler reopened this Dec 26, 2016
@sfackler
Copy link
Member

cc @rust-lang/libs

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 26, 2016
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks @djzin!

(Included(start), _) => Included(start),
(Excluded(start), _) => Excluded(start),
(Unbounded, _) => Unbounded,
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be self.0, right? (similarly self.1 below)

/// # }
/// ```
fn end(&self) -> Option<&T> {
None
fn end(&self) -> Bound<&T> {
Copy link
Member

Choose a reason for hiding this comment

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

While we're here could we change these to non-default methods as well? The default of Unbounded may not make as much sense as None before.

@arthurprs
Copy link
Contributor

cc #27787 as github wasn't smart enough.

@aturon
Copy link
Member

aturon commented Jan 4, 2017

Awesome, thanks for taking this on!

@alexcrichton
Copy link
Member

ping @sfackler on the review

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 14, 2017

📌 Commit bff737d has been approved by sfackler

@bors
Copy link
Contributor

bors commented Jan 14, 2017

⌛ Testing commit bff737d with merge 7631057...

@bors
Copy link
Contributor

bors commented Jan 14, 2017

💔 Test failed - status-travis

@djzin
Copy link
Contributor Author

djzin commented Jan 14, 2017

@sfackler rebased onto master, fixing up array_vec errors

@alexcrichton
Copy link
Member

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Jan 15, 2017

📌 Commit bd04c30 has been approved by sfackler

@bors
Copy link
Contributor

bors commented Jan 15, 2017

⌛ Testing commit bd04c30 with merge 0ef85a6...

bors added a commit that referenced this pull request Jan 15, 2017
Implementation of plan in issue #27787 for btree_range

Still some ergonomics to be worked on, the ::<str,_> is particularly unsightly
@bors
Copy link
Contributor

bors commented Jan 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 0ef85a6 to master...

@bors bors merged commit bd04c30 into rust-lang:master Jan 15, 2017
0x7CFE added a commit to 0x7CFE/cortex that referenced this pull request Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants