Skip to content

Fix reverse of ranges of unsigned integers#29842

Merged
oscardssmith merged 1 commit intomasterfrom
mb/reverseunsignedrange
Jan 6, 2022
Merged

Fix reverse of ranges of unsigned integers#29842
oscardssmith merged 1 commit intomasterfrom
mb/reverseunsignedrange

Conversation

@mbauman
Copy link
Copy Markdown
Member

@mbauman mbauman commented Oct 29, 2018 •
edited by vtjnash
Loading

Fixes #29576, works around improves #29810.

Edit: well, works around might be a little strong. It changes a segfault to an inexact error, so this doesn't add support for reversing ranges of pointers (hence the lack of tests thereof).

Closes #42718

@chethega
Copy link
Copy Markdown
Contributor

chethega commented Oct 29, 2018

Cool, inexact is very preferable to segfault, and your PR points out the problem:

function unsafe_length(r::StepRange)
    n = Integer(div((r.stop - r.start) + r.step, r.step))
    isempty(r) ? zero(n) : n
end

should maybe become

function unsafe_length(r::StepRange)
    if r.step > 0
        n = Integer(div((r.stop - r.start) + r.step, r.step))
    else
       n = Integer(div(-(r.stop - r.start) - r.step, -r.step))
    end
    isempty(r) ? zero(n) : n
end

which would fix the pointer range. On the other hand, this still smells very brittle. Do we need to support ranges with larger than Int64 length?

@vtjnash vtjnash force-pushed the mb/reverseunsignedrange branch from bf40245 to 68afe07 Compare November 11, 2021 18:52
@vtjnash vtjnash added the triage This should be discussed on a triage call label Nov 11, 2021
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Nov 11, 2021

I've expanded this more, and think we should merge, but adding triage label in case someone wants to contest it. I noticed after pushing this that there might also be a case in intersect that I missed switching from -step to negate(step), which might be good to add here too.

@vtjnash vtjnash mentioned this pull request Nov 11, 2021
Closed
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Nov 11, 2021

:D

Error in testset sorting:
Error During Test at /buildworker/worker/tester_linux32/build/share/julia/test/sorting.jl:220
 Unexpected Pass
 Expression: length(reverse(0x01:0x02)) == 2
 Got correct result, please change to @test if no longer broken.

@mbauman @vtjnash @LilithHafner
Fix reverse of ranges of unsigned integers
f7e3259 
Fixes #29576

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
@vtjnash vtjnash force-pushed the mb/reverseunsignedrange branch from 68afe07 to f7e3259 Compare November 12, 2021 16:13
@oscardssmith
Copy link
Copy Markdown
Member

oscardssmith commented Nov 15, 2021

Is this ready to merge?

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Nov 15, 2021

It has the triage label, but we could also merge now, and then see if anyone has feedback

@oscardssmith oscardssmith merged commit 68e120f into master Jan 6, 2022
@oscardssmith oscardssmith deleted the mb/reverseunsignedrange branch January 6, 2022 20:19
@oscardssmith oscardssmith removed the triage This should be discussed on a triage call label Jan 6, 2022
vtjnash added a commit that referenced this pull request Jan 13, 2022
@vtjnash
fix ranges PR conflict
f12f315 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between #43059 and #29842. And this is likely clearer anyways.

Closes #43788
@vtjnash vtjnash mentioned this pull request Jan 13, 2022
Merged
vtjnash added a commit that referenced this pull request Jan 13, 2022
@vtjnash
fix ranges PR conflict (#43790)
b351286 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between #43059 and #29842. And this is likely clearer anyways.

Closes #43788
nickrobinson251 pushed a commit to nickrobinson251/julia that referenced this pull request Jan 14, 2022
@vtjnash @nickrobinson251
fix ranges PR conflict (JuliaLang#43790)
6fbd2c4 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways.

Closes JuliaLang#43788
nickrobinson251 pushed a commit to nickrobinson251/julia that referenced this pull request Jan 14, 2022
@vtjnash @nickrobinson251
fix ranges PR conflict (JuliaLang#43790)
577b4ea 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways.

Closes JuliaLang#43788
LilithHafner added a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@mbauman @vtjnash @LilithHafner
Fix reverse of ranges of unsigned integers (JuliaLang#29842)
eb7bea1 
Fixes JuliaLang#29576
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@vtjnash @LilithHafner
fix ranges PR conflict (JuliaLang#43790)
667e646 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways.

Closes JuliaLang#43788
LilithHafner added a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@mbauman @vtjnash @LilithHafner
Fix reverse of ranges of unsigned integers (JuliaLang#29842)
e603b25 
Fixes JuliaLang#29576
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@vtjnash @LilithHafner
fix ranges PR conflict (JuliaLang#43790)
74e34ec 
We cannot safely use reverse, so we do not anymore. That is now causing
conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways.

Closes JuliaLang#43788
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

reverse of an unsigned range does not work as expected

4 participants

@mbauman @chethega @vtjnash @oscardssmith