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

Allow Char as a needle for contains functions #22435

Merged
merged 1 commit into from
Jun 24, 2017
Merged

Conversation

musm
Copy link
Contributor

@musm musm commented Jun 19, 2017

close #22418

@musm
Copy link
Contributor Author

musm commented Jun 19, 2017

Actually it may be better to let
contains(str::AbstractString, char::Char) = search(c, s) != 0
as that is faster.

FYI in(c::Char, s::AbstractString) = (search(s,c)!=0)
so this amendment would be equivalent to in, but it seems an unnecessary restriction to only allow AbstractString needles . (I personally ran into this when I was using contains, I wasn't ware that in accepted searching with a Char)

@JeffBezanson
Copy link
Member

It looks to me like search and searchindex do the same thing in this case, so ideally they should be equally fast. Separate issue, but maybe we can fix that.

@musm
Copy link
Contributor Author

musm commented Jun 19, 2017

It looks to me like search and searchindex do the same thing in this case, so ideally they should be equally fast. Separate issue, but maybe we can fix that.

They do the same but the code they call is different. The search function with a Char is a lot simpler than the general _searchindex function. Would probably need to update _searchindex for a fast path for the Char case (probably just call the search function in this case...

julia> @btime searchindex("Saasdfasx", 'x')
  24.475 ns (0 allocations: 0 bytes)

julia> @btime search("Saasdfasx", 'x')
  18.948 ns (0 allocations: 0 bytes)


julia> @btime searchindex("Saasdfasx", "x")
  26.448 ns (0 allocations: 0 bytes)


julia> @btime search("Saasdfasx", "x")
  30.791 ns (0 allocations: 0 bytes)

# note searchindex with a Char is a lot slower than the search function
# the opposite is true when the needle is a String as expected

@andyferris
Copy link
Member

If String is iterable, I would expect to use in(::Char, ::String) (...but possibly in addition to contains which might be more convenient but as an alias for in?).

@musm
Copy link
Contributor Author

musm commented Jun 20, 2017

I opened PR #22436 to fix the perf issue so that
in
contains
will be equivalent in terms of performance with Char needles, so that we can rely on the same implementation for the contains function for these two cases.

If String is iterable, I would expect to use in(::Char, ::String) (...but possibly in addition to contains which might be more convenient but as an alias for in?).

I was testing other string containment and then tried to pass a Char and it threw an error, I didn't realize in was meant for this case, but nonetheless it seems like an unnecessary restriction on the contains function and it's easy enough to support...

This PR should be g2g now

@KristofferC KristofferC merged commit 6ced741 into JuliaLang:master Jun 24, 2017
@musm musm deleted the cont branch June 24, 2017 18:23
DrTodd13 pushed a commit to IntelLabs/julia that referenced this pull request Jun 26, 2017
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.

contains does not allow Char needle
5 participants