Skip to content

Add ManagedMap::range. #12

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

Merged
merged 3 commits into from
May 24, 2018
Merged

Conversation

progval
Copy link
Contributor

@progval progval commented May 17, 2018

@progval progval force-pushed the managedmap-range branch from b65821b to 3845cbc Compare May 17, 2018 05:09
@progval
Copy link
Contributor Author

progval commented May 17, 2018

Hmm, I'm having an issue with Rust versions. I have to do use alloc::Bound::{Included,Excluded}; on Rust < 1.26, and use core::ops::{Included,Excluded}; on Rust >= 1.26.

Any idea on how to solve this?

@progval progval force-pushed the managedmap-range branch 3 times, most recently from 56be1ed to 4ca776e Compare May 17, 2018 05:40
@progval
Copy link
Contributor Author

progval commented May 17, 2018

Got it!

src/map.rs Outdated
@@ -90,6 +96,103 @@ impl<T> Into<Option<T>> for RevOption<T> {
}
}

pub enum Range<'a, 'b, K: 'a, V: 'a> where 'a: 'b {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there a reason the lifetimes have to be like this? Why not K: 'b, V: 'b?

src/map.rs Outdated
/// Owned variant, only available with the `std` or `alloc` feature enabled.
#[cfg(any(feature = "std", feature = "alloc"))]
Owned(BTreeRange<'b, K, V>),
_Phantom(&'a K),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should, at least, be #[doc(hidden)], and I usually go with __Phantom (two underscores).

src/map.rs Outdated
break;
}
}
assert_eq!(left, right);
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this assertion fail?

src/map.rs Outdated
break;
}
}
assert_eq!(left, right);
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this assertion fail?

src/map.rs Outdated
let middle = left + (right-left)/2;
if key!(slice[middle]) < Some(key_begin) {
left = middle+1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: } else here and elsewhere.

src/map.rs Outdated
@@ -155,6 +258,23 @@ impl<'a, K: Ord + 'a, V: 'a> ManagedMap<'a, K, V> {
}
}

pub fn range<'b, 'c, Q>(&'c self, key_begin: &Q, key_end: &Q) -> Range<'a, 'b, K, V>
where K: Borrow<Q>, Q: Ord + ?Sized, 'a: 'b, 'c: 'a
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 'c really necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the ManagedMap has to outlive the Range's references.

src/map.rs Outdated
break;
}
}
assert_eq!(left, right);
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this really was a question. Will this assertion fail if the entries are out of order or something like that? Then it should stay. See the other assertions in this file that panic with "invariant violated".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it can't fail. I used it while debugging

@whitequark
Copy link
Contributor

Thanks. What do you think about removing rustc version detection entirely? I was thinking about using impl Trait in smoltcp and that would imply 1.26. I don't think we care very much about 1.25 or earlier.

@progval
Copy link
Contributor Author

progval commented May 17, 2018

Okay. I currently have only rust 1.25 on my computer, but I can fix that.

@progval progval force-pushed the managedmap-range branch 2 times, most recently from 5b7dfe7 to 7849e64 Compare May 17, 2018 08:59
@progval progval force-pushed the managedmap-range branch from 7849e64 to 63e49b3 Compare May 17, 2018 17:32
@progval
Copy link
Contributor Author

progval commented May 17, 2018

I wonder if I should add "shortcuts", to skip a dichotomy if key_begin <= slice[0] or key_end > slice[slice.len()-1].

smoltcp-rs/smoltcp#209 would be one use case, because we'll use 0.0.0.0/0 or ::/0 as the key_begin.

@progval
Copy link
Contributor Author

progval commented May 19, 2018

I just improved my implementation to have an interface similar to BTreeMap::range, and support more range types than "left included, right excluded". (we'll need to include right in smoltcp.)

It uses the nightly feature "collections_range", but it is going to be stabilized soon; and ManagedMap already depends on "slice_rotate" anyway.

@whitequark whitequark merged commit 99e4395 into smoltcp-rs:master May 24, 2018
@whitequark
Copy link
Contributor

Thanks.

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

Successfully merging this pull request may close these issues.

2 participants