-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
expose findfirst findnext for UInt8 vector #37283
Conversation
3cd123d
to
847d58b
Compare
Should these be moved away from the string file now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This makes sense. I have a few remarks:
- It sounds like this method should eventually work for any
AbstractVector
. The fact that onlyUInt8
/Int8
are supported is due to an implementation detail. It's probably OK to start with a restricted method though, and generalize it later. - We should measure the consequences of this addition for the API if we want to generalize this. With this PR,
findfirst(pattern, collection)
will mean "look for the sequencepattern
incollection
". Currently, we havefindfirst(pred::Function, A)
,findfirst(ch::AbstractChar, string::AbstractString)
andfindfirst(pattern::AbstractString, string::AbstractString)
. The methods added by this PR are consistent with the third method, i.e. the two arguments collections, and we are looking for a sequence. Do note that it is at odds with the second one, where the first argument is a single element. This is OK for strings as there's no ambiguity, but in general withfindfirst(x::AbstractArray, A::AbstractArray)
you cannot know whetherx
is supposed to be a sequence or an element sinceA
could be anAbstractArray{<:AbstractArray}
. So if we go with this PR, we will renounce to the possibility to treatx
as a single element, e.g. we'll have to keep writingfindfirst(==(1), [1, 2])
. I think that's OK since the syntax is relatively short, but let's be aware of this decision. - Finally,
findlast
andfindprev
should probably be implemented in this PR for consistency (I think_rsearch
allows doing this).
Not as long as this is restricted to a small set of element types. I agree that this will come up in a hypothetical future PR where we generalize this to |
9fe84a2
to
6439107
Compare
As suggested by @stevengj , shrinking original |
@nalimilan I have added |
300c4df
to
f073830
Compare
Took the suggested approach by @stevengj : shifting |
032b93c
to
2d4b684
Compare
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
d303612
to
ff679b0
Compare
I believe this can be merged now (before another news merge conflict occur) |
shorten NEWS
Thanks! |
When squashing and merging these commits, please try to fix up the commit message to contain only the relevant information. This one just had a rather long list of repeated:
and it makes |
fix #37280