Skip to content

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

Closed
sanxiyn opened this issue Mar 7, 2013 · 4 comments
Closed

Change range_rev to be inclusive on the low end #5270

sanxiyn opened this issue Mar 7, 2013 · 4 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@sanxiyn
Copy link
Member

sanxiyn commented Mar 7, 2013

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 since hi and lo should be the same type, this can't be uint, 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.

@yjh0502
Copy link
Contributor

yjh0502 commented Mar 8, 2013

How about range_rev(lo, hi) which iterates over (hi..lo]? For example, range_rev(0, 10) => [9, ..., 0]

@pnkfelix
Copy link
Member

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).

@pnkfelix
Copy link
Member

Well, there is actually the issue that both range and range_rev are implemented in terms of range_step, which is easy right now because they both traverse a range of the form [x, y). If we make this change, it might be easier to make things work if they are implemented atop a function that traverses a range of the form [x,y], rather than having to handle such different cases of [x,y) and (x,y] within a single routine. (I.e. generate the inclusive limit value y from the original stop by subtracting step from stop.) Or maybe this is a bad idea overall.

@ghost ghost assigned pnkfelix Jun 27, 2013
@pnkfelix
Copy link
Member

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.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 1, 2013
pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 11, 2013
bors added a commit that referenced this issue Jul 16, 2013
…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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
3 participants