-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
New API: Range::cmp_scalar; comparison (less/equal/greater) to a primitive of the Range #102343
base: master
Are you sure you want to change the base?
Conversation
…itive of the Range
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
An ACP (API change proposal) is required. See https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html. @rustbot label +T-libs-api -T-libs +S-waiting-on-ACP |
Oh, there was the "API change proposal" thing. Thanks for the pointer. I'll send one later this week! |
I mentioned this in the ACP, but apparently |
…ress degenerate ranges. Added simple examples to docs.
☔ The latest upstream changes (presumably #102632) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
@golddranks if you can resolve the conflicts, we can push this forward also reässigning |
I'm trying to reactivate on this! |
@golddranks |
ACP: rust-lang/libs-team#115
This PR adds a method,
cmp_scalar
toRange
,RangeFrom
,RangeTo
,RangeInclusive
andRangeToInclusive
.The point of the method is to compare the range to a single
Ord
-comparable element of the range (I call it "scalar" here, because I think that makes sense for an element that implementsOrd
.); the method returnsOrdering::{Less, Greater, Equal}
, depending on whether the range is below or above the element, or contains it.Justification
Simple and obvious operation to do
Given a range and a number (or anything that implements
Ord
), beyond the "is contained" check, comparing whether the number is above or below the range is a simple and obvious operation to do, and possibly the only operation that makes sense in this generic form of having a range ofT: Ord
s.Useful when searching amongst multiple ranges
The example given in the doctests gives a clear motivation for this API; a search through multiple ranges with a specified target:
Why compare a range to a scalar, and not scalar to range?
I.e. why i.e.
(3..9).cmp_scalar(5)
instead of5.cmp_range(3..9)
?Originally, I thought that an API to compare a number to a range would make more sense intuitively, and set out to implement it. However, it turned out that there were two kinds of problems:
The order in the original motivation gets reversed, and
reverse()
must be called after this operation to fix it:files.binary_search_by(|f| target.cmp_range(f.range)).reverse()?;
Of course,
cmp_range
would make sense, if searching for a single number among of many, that fits the range. However, I had hard time imagining a common case where this would be the requirement, whereas the motivation forcmp_scalar
was clear from the case introduced above.The implementation gets gnarly, as the API must take the range as a generic parameter, and the different kind of Ranges are different types. I tried to implement it as generic
T: Into<RangeBounds>
, but the ergonomics kind of sucked. Also, the problem of ranges not beingCopy
could be mitigated when the range is passed as the&self
parameter because auto-ref works in that position.Points for discussion
cmp_scalar
make sense? To me it kind of does, but I see no precedent with calling stuff "scalars" in the stdlib. I wonder if there are better alternatives.Ord
vsPartialOrd
? Withcontains
,PartialOrd
makes sense, but with a proper ordering returned by this API, I think thatOrd
is the only sensible choice.&str
vsString
comparisons, for example)? That might worsen the ergonomics because it makes type inference harder.