Support RangeBounds for T: Comparable

[al8n]

Hi, this PR adds functionality like RangeBounds::contains for T: Comparable.

[cuviper]

I think having a separate trait will make this awkward to use, because that's an additional constraint on the range type that you'd have to propagate in API. How about something like this added to Comparable (with a default impl):

    fn compare_contains<R: RangeBounds<Self>>(range: R, key: &K) -> bool;

It will be more awkward to use without the self receiver though, so I'm not sure this is better -- just an alternative to consider.

This would also keep the "direction" consistent, like my other comment.

[cuviper]

What is the intended use case? If it's something like BTreeMap::range, then I think the relationship should be the other way, T: Comparable<U>, or in map terms it's RangeBounds<Q> and Q: Comparable<K>.

[al8n]

The intended use case is most like a BTreeMap, I need something like

fn foo<R, Q>(&self,  key: &K) 
where
  R: RangeBounds<Q>,
  Q: ?Sized + Comparable<K>
{
  if self.range.comparable_contains(key) { }
}

[al8n]

I think having a separate trait will make this awkward to use, because that's an additional constraint on the range type that you'd have to propagate in API. How about something like this added to Comparable (with a default impl):

    fn compare_contains<R: RangeBounds<Self>>(range: R, key: &K) -> bool;

It will be more awkward to use without the self receiver though, so I'm not sure this is better -- just an alternative to consider.

This would also keep the "direction" consistent, like my other comment.

Yeah, another trait will require users to import it, but if there is no self receiver, the code should be something like Comparable::compare_contains(range, key). It is hard to assess which one is better.

[cuviper]

Yeah, another trait will require users to import it,

Hmm, my thought was more that you would have to add the trait as a constraint in your public API, but I guess that's not true -- with the blanket impl, you can still use it as long as RangeBounds and Comparable are met independently. So this is really a pure extension trait for convenience, and doesn't need any API-visible change.

but if there is no self receiver, the code should be something like Comparable::compare_contains(range, key).

Yes, or I'd write Q::compare_contains(range, key). But with my realization above, I think your way is better.

[cuviper]

I pushed a few updates myself.

I'm still hesitating whether this really makes sense as <Range<Q>>::compare_contains<K> though, and not the other way around for Q and K. The current way could make sense if the user is providing a Q-based range for a collection of K.

However, your example looks like your user is providing a &K, and that's usually not what we want. The pattern is usually an owned K compared to a borrowed Q, like K = String and Q = str, so user queries with Q benefit from avoiding allocation.

[cuviper]

If you have a real-world example in public code, that would really help me see what you're doing. I know it probably seems really nitpicky, but this is a 1.0 crate in the public API of other crates, so bumping semver is costly -- I don't want to add something we'll regret. (like #5)

If we can figure out decent names for either direction, we could perhaps have it both ways, like:

pub trait ComparableRangeBounds<T: ?Sized>: RangeBounds<T> {
    fn compare_contains_k<K>(&self, item: &K) -> bool
    where
        K: ?Sized,
        T: Comparable<K>;
    fn compare_contains_q<Q>(&self, item: &Q) -> bool
    where
        Q: ?Sized + Comparable<T>;
}

[al8n]

Hi, although, crossbeam-skiplist currently is not using equivalent, here is an example of how the compare_contains may work. In crossbeam-skiplist, if there is a compare_contains, then those two functions can be removed.