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: add find(first|next|prev|last) variants with nothing sentinel #457

Closed
wants to merge 1 commit into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 13, 2018

This will allow Compat.findnext to use the new nothing sentinel.

I'm frankly not sure what to do about several of the aspects of this PR, particularly the string-related ones. I'm also not sure we need the SparseArray changes in Compat? Advice appreciated (or help from anyone who wants to take it from here).

@fredrikekre
Copy link
Member

I'm also not sure we need the SparseArray changes in Compat

Those where added just recently and are not there on 0.6, should be fine to use the slower fallbacks on 0.6.

@fredrikekre
Copy link
Member

fredrikekre commented Jan 15, 2018

AFAICT all of those returned 0 previously? Could the implementations here not simply be

if VERSION < v"0.7.0-something"
    function findnext(A, start::Integer)
        i = Base.findnext(A, start)
        if i == 0
            return nothing
        end
        return i
    end
else
    const findnext = Base.findnext
end

@timholy
Copy link
Member Author

timholy commented Jan 15, 2018

That would be a little dangerous for the day when I want to do this:

a = OffsetArray([false, true, false], -1:1)
Compat.findfirst(a)

and expect it to work properly.

@nalimilan
Copy link
Member

Wow! Thanks for doing this radical backporting work!

However, to be honest, I'm hesitant about whether it's a good idea to copy all of these functions. @fredrikekre's proposal is appealing for its simplicity. All this duplicated code will have to be kept in sync with Base, which is tedious (I have already a few new PRs open) and which we will probably forget to do. Are you really concerned about people using the brand new OffsetArrays support on Julia 0.6? By the time users will have discovered it exists, we'll have dropped support for that release, and the error is relatively easy to catch anyway.

@garrison
Copy link
Member

Have these methods stabilized enough in Base that they could be brought in here? I don't have a strong opinion on whether they should be copied or the simpler approach should be taken, but either way it would be great to come to a decision soon so porting can continue.

@nalimilan
Copy link
Member

Actually this has been covered by #484 and #513.

@nalimilan nalimilan closed this Mar 31, 2018
@timholy timholy deleted the teh/findnext branch April 20, 2018 14:55
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.

4 participants