-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Change range_rev to be inclusive on the low end #5270
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
Comments
How about |
Not sure if I understand what "I-wrong" is supposed to denote. Anyway, this seems easy to fix (apart from bikeshed'ing the argument order). |
Well, there is actually the issue that both |
I think I have a pretty clean way to put this in. I'll have a patch up for review shortly. Assigning bug to self. |
…ss-issue5270-2ndpr, r=cmr Changes int/uint range_rev to iterate over range `(hi,lo]` instead of `[hi,lo)`. Fix #5270. Also: * Adds unit tests for int/uint range functions * Updates the uses of `range_rev` to account for the new semantics. (Note that pretty much all of the updates there were strict improvements to the code in question; yay!) * Exposes new function, `range_step_inclusive`, which does the range `[hi,lo]`, (at least when `hi-lo` is a multiple of the `step` parameter). * Special-cases when `|step| == 1` removing unnecessary bounds-check. (I did not check whether LLVM was already performing this optimization; I figure it would be a net win to not leave that analysis to the compiler. If reviewer objects, I can easily remove that from the refactored code.) (This pull request is a rebased version of PR #7524, which went stale due to recent unrelated changes to num libraries.)
Currently
range_rev(hi, lo)
iterates[hi..lo)
, but it probably should iterate(hi..lo]
.For example, consider iterating indexes of vector of length 10 from the end. Currently that is
range_rev(10-1, -1)
, but sincehi
andlo
should be the same type, this can't beuint
, and to be used as an index it needs to be cast.range_rev(10, 0)
would be better.This is from the mailing list discussion.
The text was updated successfully, but these errors were encountered: