Skip to content
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

Fix reverse of ranges of unsigned integers #29842

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Oct 29, 2018

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
Contributor

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 added the triage This should be discussed on a triage call label Nov 11, 2021
@vtjnash
Copy link
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
Copy link
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.

Fixes #29576

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
@oscardssmith
Copy link
Member

Is this ready to merge?

@vtjnash
Copy link
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
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
vtjnash added a commit that referenced this pull request Jan 13, 2022
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
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
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 pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
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
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 pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reverse of an unsigned range does not work as expected
4 participants