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

make find functions sign-sensitive #40007

Closed
wants to merge 2 commits into from

Conversation

bicycle1885
Copy link
Member

Fix #40006

nothing_sentinel(_search(a, pred.x, i))

function _search(a::Union{String,ByteArray}, b::Union{Int8,UInt8}, i::Integer = 1)
function _search(a::Union{String,ByteArray}, b::Byte, i::Integer = 1)
if i < 1
throw(BoundsError(a, i))
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively?

Suggested change
end
end
typemin(eltype(a)) <= b <= typemax(eltype(a)) || return 0

Copy link
Member Author

@bicycle1885 bicycle1885 Mar 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestion. That looks more elegant. However, I'd like to fix the problem in an upcoming pull request. I'm preparing an overhaul of findnext and friends to improve the consistency and performance, which can be found at https://github.com/bicycle1885/StringSearch.jl. This pull request is a stepping stone and the main purpose is to file an issue I found and add some test to make the next one less complicated. So, if this is acceptable, can you merge as is?

@@ -1,5 +1,8 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

const Byte = Union{Int8,UInt8}
const ByteArray{T<:Byte} = Vector{T}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be Union{Vector{UInt8},Vector{Int8}}; otherwise it includes Vector{Union{Int8,UInt8}}.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. Thank you. Fixed.

@bicycle1885
Copy link
Member Author

This pull request is superseded by #40120.

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.

findfirst (findnext) is not sign-sensitive
3 participants