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

WIP: Boundschecks for searchsorted & remove step(r::Range)==0 tests #7078

Merged
merged 1 commit into from
Jun 3, 2014

Conversation

mauro3
Copy link
Contributor

@mauro3 mauro3 commented Jun 2, 2014

  1. Checks bounds on the more low-level searchsorted* methods. At the moment
    an error like this is possible:
julia> searchsortedfirst([1:10], 5,  -10, 7, Base.Order.Forward)
zsh: segmentation fault (core dumped)

I corrected this by adding a bounds check. The alternative would be to rename the low-level searchsorted* to, say, _searchsorted* and not export them. (my understanding is that no exported methods in Base should seg-fault unless they are labeled unsafe_)

TODO: The same goes for the low-level select! and sort! methods, which also do not check bounds. But some of them are recursive and a bounds check is only necessary on the first execution. Maybe renaming would be better? @StefanKarpinski, I think you updated sort.jl most recently, maybe you can comment. Let me know what would be the preferred solution and I can update this PR.

Here an example of a sort! bug:

julia> a = rand(2)
2-element Array{Float64,1}:
 0.0272557
 0.200126 

julia> sort!(a, 1, 15, Base.Sort.QuickSort, Base.Order.Forward)
2-element Array{Float64,1}:
 9.88131e-324
 9.88131e-324
  1. Removes the step(r)==0 tests as ranges cannot have 0 steps (I think)

1) Check bounds on the more low-level searchsorted* methods.  Before
an error like this was possible:
```
julia> searchsortedfirst([1:10], 5,  -10, 7, Base.Order.Forward)
zsh: segmentation fault (core dumped)
```

2) Removes the step(r)==0 tests as ranges cannot have 0 steps (I
   think)

Note that the low-level sort! methods also do not check bounds.
Probably this should also be rectified.
@kmsquire
Copy link
Member

kmsquire commented Jun 2, 2014

Even though those particular function are not documented and not expected to be called by users, this is still the right thing to do. Good catch!

@mauro3
Copy link
Contributor Author

mauro3 commented Jun 2, 2014

Thanks Kevin.

I cannot reproduce the Travis failure on my local clang build, so not sure what that is about.

@JeffBezanson
Copy link
Member

Yes, the lack of bounds checks was due to these being internal forms of functions. It also used to be possible to have ranges with step 0.

I think we are fine with the unsafe, undocumented internal forms of sort!. This change is fine though.

JeffBezanson added a commit that referenced this pull request Jun 3, 2014
WIP: Boundschecks for searchsorted & remove step(r::Range)==0 tests
@JeffBezanson JeffBezanson merged commit 2d5dbf3 into JuliaLang:master Jun 3, 2014
@mauro3 mauro3 deleted the sort_bounds_check branch June 3, 2014 08:07
JeffBezanson referenced this pull request Aug 8, 2014
This reverts commit 10e51e5.

Originally merged by 2d5dbf3, this is
incorrect in a number of ways.

The searchsorted* methods with explicit lo and hi indices are an not
part of the public API for these functions. Thus, the correctness of
the bounds given don't need to be checked – that is up to the caller.
If we decide to expose the ability to call searchsorted* with explicit
start and end indices, there should be a better API for that which
doesn't require passing an Ordering object and which checks bounds.

In particular, this broke the old behavior of searchsorted when the
value that's being searched for is not in the array. Fix #7916. Also
added somes tests to make sure this doesn't regress again.

At the time this was committed, I think that ranges could actually
have a zero step – there was a reason for that check to be there.
This may no longer be the case, but since it's cheap to check, we may
as well handle it correctly and decouple the correctness of the
sorting functions from a detail of the range implementation.

Conflicts:
	base/sort.jl
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.

3 participants