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.

Sign up for GitHub

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

Jump to bottom

Replace use of removed Base.nothing_sentinel #28

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Replace use of removed Base.nothing_sentinel #28

merged 1 commit into from
Oct 9, 2024

Conversation

jakobnissen
Copy link
Contributor

I don't like the code duplication of the string search routines in Base:

  1. It's needless duplication that needs to be kept up with - for example, the bugfixes of Various fixes to byte / bytearray search JuliaLang/julia#54579 now has to be replicated
  2. It has all the issues outlined in Add a concept of memory-backed contiguous collection in Base JuliaLang/julia#54581 - such as the following bug:
julia> isnothing(findfirst(==('a'), StringView(codeunits(String7("abcd"))))))
true

@jakobnissen
Replace use of removed Base.nothing_sentinel
497bdff 
A better solution is to express Base's search functionality in terms of
searching over chunks of contiguous memory, as I have proposed elsewhere.
However, that will require a memory view API in Base.
@jakobnissen jakobnissen mentioned this pull request Oct 8, 2024
Merged
@stevengj
Copy link
Member

stevengj commented Oct 9, 2024

I don't like it either, but unfortunately it's not "needless" until the Base code is refactored (or at least, widen the method signatures) to allow us to call it. It would be great to see a PR to Base that allows us to delete some of this.

@stevengj stevengj merged commit 6f9020c into JuliaStrings:main Oct 9, 2024
9 checks passed
@jakobnissen jakobnissen deleted the search12 branch October 9, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jakobnissen @stevengj