Make searchsorted*/findnext/findprev return values of keytype #32978

sostock · 2019-08-20T11:35:18Z

The returntype of searchsorted* is now always the keytype of the container (and does not depend on the type of the value being searched).
The returntype of findnext/findprev is now always the keytype of the container or Nothing (and does not depend on the type and value of the start argument).

Even though strings don’t have a keytype, I changed the respective findnext/findprev methods as well. They now always return values of type Union{Int,Nothing} or Union{UnitRange{Int},Nothing}.

mbauman

This is a great PR, thank you! It looks like we need keytype defined a bit earlier in the bootstrapping order… but I don't fully understand why because to my eyes it looks like it should have already been defined at the point it errors.

mbauman · 2019-08-20T14:47:06Z

base/bitarray.jl

@@ -1385,7 +1385,7 @@ end
 function findnext(B::BitArray, start::Integer)
    start > 0 || throw(BoundsError(B, start))
    start > length(B) && return nothing
-    unsafe_bitfindnext(B.chunks, start)
+    unsafe_bitfindnext(B.chunks, Int(start))


unsafe_bitfindnext (and prev, too) accepts a start::Integer. I was about to make a comment that we should just move this there and/or restrict its signature, but then I realized it's also used by BitSet. BitSet demands an Int64 result, whereas its use for BitArray demands an Int result. So this seems like the right way to go about it. 👍

Maybe it changed, but my impression is that BitSet calls unsafe_bitfindnext by feeding it an Int, not an Int64, so I would also favor restricting unsafe_bitfindnext to Int input, as this is an internal function. But the PR is also fine as is!

I have now restricted unsafe_bitfindnext to Int.

Ok, sorry for the nitpick, but please change also unsafe_bitfindprev accordingly too :)

That was fast!

base/sort.jl

sostock · 2019-08-20T17:48:02Z

I also encountered the bootstrap error during development, although I managed to build Julia successfully in the end by applying my changes incrementally. I tried to debug it (following the documentation), but I couldn’t even get the build process running in gdb.

Due to the bootstrap issue, I used oftype(l, start) instead of convert(keytype(A), start) in base/array.jl. Once the bootstrap issue is resolved, this could be changed back (although it probably doesn’t make a difference).

sostock · 2020-02-03T08:23:52Z

I just tried to debug this again (following the documentation), but it does not seem to be working. Unfortunately, I have no experience with gdb, so I don’t know what could be done about this.

GNU gdb (GDB) 8.3.1
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
/usr/lib/../share/gcc-9.2.0/python/libstdcxx/v6/xmethods.py:731: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
  refcounts = ['_M_refcount']['_M_pi']
ERROR: unknown option `--build`
[Inferior 1 (process 5168) exited with code 01]
../contrib/debug_bootstrap.gdb:3: Error in sourced command file:
No stack.
(gdb)

rfourquet · 2020-04-21T11:12:39Z

@sostock thanks for this great PR! I believe you can fix the compilation issue by adding keytype to the list of imported functions in "sort.jl" (in the list starting with using .Base: copymutable, ... at the top). At least I managed to build your branch this way, was there another issue?

sostock · 2020-04-21T20:36:58Z

Thanks, @rfourquet! Importing keytype works for me as well. I think there are no other issues with this PR.

rfourquet

LGTM, modulo the seemingly unnecessary change to searchsortedfirst testing for x isa Unsigned.

rfourquet · 2020-04-23T13:43:56Z

base/bitarray.jl

@@ -1385,7 +1385,7 @@ end
 function findnext(B::BitArray, start::Integer)
    start > 0 || throw(BoundsError(B, start))
    start > length(B) && return nothing
-    unsafe_bitfindnext(B.chunks, start)
+    unsafe_bitfindnext(B.chunks, Int(start))


Maybe it changed, but my impression is that BitSet calls unsafe_bitfindnext by feeding it an Int, not an Int64, so I would also favor restricting unsafe_bitfindnext to Int input, as this is an internal function. But the PR is also fine as is!

rfourquet · 2020-04-23T13:44:02Z

base/bitarray.jl

@@ -1411,8 +1411,9 @@ function findnextnot(B::BitArray, start::Integer)
    l = length(Bc)
    l == 0 && return nothing

-    chunk_start = _div64(start-1)+1
-    within_chunk_start = _mod64(start-1)
+    st = Int(start)


As an aside, note that for this kind of thing, there is no problem writing start = Int(start), which saves you from having to find another name :)

Ok, then I will change it.

rfourquet · 2020-04-23T13:46:28Z

base/sort.jl

@@ -285,14 +285,15 @@ function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderi
        lastindex(a) + 1
    else
        if o isa ForwardOrdering
-            -fld(floor(Integer, -x) + first(a), h) + 1
+            y = isa(x, Unsigned) ? floor(-Signed(x)) : floor(Integer, -x)


When x isa Unsigned, this method looks like it won't be called, but rather the next method below (I guess this split didn't exist when you put up this PR).

Yes, the method below is used for x::Unsigned. I have restored the original, except for using Signed(first(a)) instead of first(a) (which is needed for passing the tests added in this PR).

rfourquet · 2020-04-23T13:51:53Z

base/strings/search.jl

@@ -130,7 +130,7 @@ function findnext(testf::Function, s::AbstractString, i::Integer)
    @inbounds i == z || isvalid(s, i) || string_index_err(s, i)
    for (j, d) in pairs(SubString(s, i))
        if testf(d)
-            return i + j - 1
+            return Int(i + j - 1)


As a conversion is needed somewhere in this function, my personal preference would favor converting at the begining (i = Int(i), which might help having to compile less function specializations, like isvalid or SubString, but this also might not matter). But keep it as you prefer.

Ok, I have changed it.

rfourquet · 2020-04-24T09:43:18Z

base/sort.jl

@@ -285,14 +285,14 @@ function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderi
        lastindex(a) + 1
    else
        if o isa ForwardOrdering
-            -fld(floor(Integer, -x) + first(a), h) + 1
+            -fld(floor(Integer, -x) + Signed(first(a)), h) + 1


So the Signed here is for the case where x is not an Integer ? Just by curiosity, without this conversion here, the type conversion attached to the return value of the function (::keytype(a)) is not enough?

No, it is for the case when x is Signed and first(a) is Unsigned. For example, if floor(Integer, -x) == Int64(-5) and first(a) == UInt64(1), adding them yields the huge number 0xfffffffffffffffc (the negative Int64 gets promoted to UInt64). The result of the complete line is then a UInt64 that is too large to convert to keytype(a).

Ah right I was off as Signed is not applied to x, and ok, the ::keytype(a) fails then, thanks.

* origin/master: (833 commits) Improve typesubtract for tuples (#35600) Make searchsorted*/findnext/findprev return values of keytype (#32978) fix buggy rand(RandomDevice(), Bool) (#35590) remove `Ref` allocation on task switch (#35606) Revert "partr: fix multiqueue resorting stability" (#35589) exclude types with free variables from `type_morespecific` (#35555) fix small typo in NEWS.md (#35611) enable inline allocation of structs with pointers (#34126) SparseArrays: Speed up right-division by diagonal matrices (#35533) Allow hashing 1D OffsetArrays NEWS item for introspection macros (#35594) Special case empty covec-diagonal-vec product (#35557) [GCChecker] fix a few tests by looking through casts Use norm instead of abs in generic lu factorization (#34575) [GCChecker,NFC] run clang-format -style=llvm [GCChecker] fix tests and add Makefile Add introspection macros support for dot syntax (#35522) Specialize `union` for `OneTo` (#35577) add pop!(vector, idx, [default]) (#35513) bump Pkg version (#35584) ...

sostock added 2 commits August 19, 2019 15:59

Make searchsorted* return values of keytype

e392400

Make findnext/findprev return keytype

5558113

mbauman reviewed Aug 20, 2019

View reviewed changes

sostock added 2 commits August 20, 2019 19:55

Use explicit return type in searchsorted

831f607

Merge branch 'master' into searchsorted_returntype

2f5ef4a

rfourquet added kind:bugfix This change fixes an existing bug domain:collections Data structures holding multiple items, e.g. sets labels Apr 21, 2020

sostock added 3 commits April 21, 2020 14:44

Resolve bootstrap issue

d820a59

Fix searchsortedfirst for unsigned integers

f55346e

Merge branch 'master' into searchsorted_returntype

d3aa626

Make tests more compact

ee5c898

rfourquet reviewed Apr 23, 2020

View reviewed changes

sostock added 4 commits April 23, 2020 20:30

Restrict unsafe_bitfindnext to Int

c666306

Rename st -> start

61abe1d

Remove unnecessary Unsigned check

57dd806

Move Int conversion

1e4db52

rfourquet reviewed Apr 24, 2020

View reviewed changes

rfourquet approved these changes Apr 24, 2020

View reviewed changes

Restrict unsafe_bitfindprev to Int

62d3586

rfourquet merged commit 6248170 into JuliaLang:master Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make searchsorted*/findnext/findprev return values of keytype #32978

Make searchsorted*/findnext/findprev return values of keytype #32978

sostock commented Aug 20, 2019 •

edited

Loading

mbauman left a comment

mbauman Aug 20, 2019

rfourquet Apr 23, 2020

sostock Apr 24, 2020

rfourquet Apr 24, 2020

sostock Apr 24, 2020

rfourquet Apr 24, 2020

sostock commented Aug 20, 2019

sostock commented Feb 3, 2020

rfourquet commented Apr 21, 2020

sostock commented Apr 21, 2020

rfourquet left a comment

rfourquet Apr 23, 2020

rfourquet Apr 23, 2020

sostock Apr 23, 2020

rfourquet Apr 23, 2020

sostock Apr 24, 2020

rfourquet Apr 23, 2020

sostock Apr 24, 2020

rfourquet Apr 24, 2020

sostock Apr 24, 2020 •

edited

Loading

rfourquet Apr 24, 2020

Make searchsorted*/findnext/findprev return values of keytype #32978

Make searchsorted*/findnext/findprev return values of keytype #32978

Conversation

sostock commented Aug 20, 2019 • edited Loading

mbauman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sostock commented Aug 20, 2019

sostock commented Feb 3, 2020

rfourquet commented Apr 21, 2020

sostock commented Apr 21, 2020

rfourquet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sostock Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sostock commented Aug 20, 2019 •

edited

Loading

sostock Apr 24, 2020 •

edited

Loading