Accept AbstractChar in count and findall (::Char, ::String) #38675

lmiq · 2020-12-03T12:44:29Z

Followup from this thread:

https://discourse.julialang.org/t/method-for-counting-characters-in-string-count-a-batman-vs-count-a-batman/51153

Seems that accepting AbstractChar in this function is enough to solve that problem.

test/regex.jl

base/regex.jl

test/regex.jl

base/regex.jl

stevengj · 2020-12-03T17:30:50Z

Needs an item in NEWS.md

bradeneliason · 2020-12-03T21:17:07Z

Checking for overlap isn't much of an overhead, but thoughts on creating a specialized method for Base.count(t::AbstractChar, s::AbstractString) that's marginally faster?

StefanKarpinski · 2020-12-04T20:36:27Z

I would measure if there's anything to be gained first. The compiler is pretty aggressive with constant propagation, so the specialization might already be happening. Even if it doesn't end up specializing at compile time, LLVM should be able to see that the overlap flag is never changed and potentially split the loop into two different versions. Even if that doesn't happen, the branch will be perfectly predictable, so there might be no cost to checking the flag.

lmiq · 2020-12-05T13:21:29Z

I benchmarked here versions with and without the overlap option, and I could not detect any difference reliably. The fluctuations on the benchmarks, even for long strings (ten sentences or so) are much greater than any possible differences in performance, if there are any.

The code_llvm of both seemed to be the same, in the sense that I could not find signs of a conditional in one case that was not present in the other, although I am not much qualified to understand that code.

StefanKarpinski · 2020-12-05T19:03:48Z

That seems definitive enough — no need to special case it.

StefanKarpinski · 2020-12-05T19:04:26Z

Btw, you've got some whitespace issues that need to be fixed.

lmiq · 2020-12-05T19:51:59Z

I think all is fixed now. Thank you for your help. I hope in the future I will be able to contribute to something more substantial and needing less feedback :-). Nice to learn about how collaborations work at this level.

StefanKarpinski · 2020-12-05T20:14:47Z

No problem at all! It's amazing how much fiddling around goes into seemingly simple changes. You have no idea how many times I've started my day thinking "well, this should be simple" only to have it take up the next several weeks 😬

lmiq · 2020-12-09T10:43:20Z

Apparently if this is to be released in 1.6, it needs a backport label.

KristofferC · 2020-12-09T10:45:14Z

New feature, doesn't go into 1.6.

lmiq · 2020-12-09T10:48:15Z

New feature, doesn't go into 1.6.

The compat entries we wrote above are of the form

!!! compat "Julia 1.6"
      Using a character as the pattern requires at least Julia 1.6.

Is that correct?

KristofferC · 2020-12-09T10:55:36Z

No, it should be changed to 1.7.

lmiq · 2020-12-09T12:32:10Z

I am not sure if @StefanKarpinski wants to comment, as he added the compat entry for the first time. Otherwise I can change that right away.

stevengj · 2020-12-09T17:16:46Z

The old compat entry was added before 1.6 branched off. We are now on 1.7-dev.

stevengj · 2020-12-09T17:17:11Z

Also needs a NEWS.md entry.

lmiq

updated compat entries

base/regex.jl

This change solves an issue raised in this post, in which searching for a char in a string results in a Method error: https://discourse.julialang.org/t/method-for-counting-characters-in-string-count-a-batman-vs-count-a-batman/51153

with overlap=true and empty string

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>

NEWS.md

Co-authored-by: Steven G. Johnson <stevenj@mit.edu>

…g#38675) This change solves an issue raised in this post, in which searching for a char in a string results in a Method error: https://discourse.julialang.org/t/method-for-counting-characters-in-string-count-a-batman-vs-count-a-batman/51153 Co-authored-by: Stefan Karpinski <stefan@karpinski.org> Co-authored-by: Steven G. Johnson <stevenj@mit.edu>

jw3126 reviewed Dec 3, 2020

View reviewed changes

test/regex.jl Outdated Show resolved Hide resolved

base/regex.jl Show resolved Hide resolved

StefanKarpinski reviewed Dec 3, 2020

View reviewed changes

base/regex.jl Show resolved Hide resolved

lmiq changed the title ~~Accept AbstractChar in method count(t, s::String)~~ Accept AbstractChar in count and findall (::Char, ::String) Dec 3, 2020

StefanKarpinski reviewed Dec 3, 2020

View reviewed changes

test/regex.jl Outdated Show resolved Hide resolved

stevengj reviewed Dec 3, 2020

View reviewed changes

base/regex.jl Outdated Show resolved Hide resolved

stevengj reviewed Dec 3, 2020

View reviewed changes

base/regex.jl Outdated Show resolved Hide resolved

stevengj added the strings "Strings!" label Dec 3, 2020

stevengj added the needs news A NEWS entry is required for this change label Dec 3, 2020

lmiq commented Dec 9, 2020

View reviewed changes

base/regex.jl Outdated Show resolved Hide resolved

base/regex.jl Outdated Show resolved Hide resolved

base/regex.jl Outdated Show resolved Hide resolved

base/regex.jl Outdated Show resolved Hide resolved

lmiq and others added 6 commits December 9, 2020 16:27

Added test for count(::Char,::String)

6a5550f

Accept Char as input to count(t,::String)

0b83779

This change solves an issue raised in this post, in which searching for a char in a string results in a Method error: https://discourse.julialang.org/t/method-for-counting-characters-in-string-count-a-batman-vs-count-a-batman/51153

Added tests for count with char as argument

289ec69

with overlap=true and empty string

Update base/regex.jl

1bcf8cf

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>

added test with non-ascii chars

cee7e88

added method findall(::Char,::String)

bdd7071

lmiq and others added 15 commits December 9, 2020 16:27

added tests for findall with Char as argument

2a5049e

Update test/regex.jl

d5e9c2a

Co-authored-by: Stefan Karpinski <stefan@karpinski.org>

removed spaces on type declarations

08afd20

update compat entry

e0d605f

fixed white spaces

0c8f92a

fixed white spaces

3c1977a

Update base/regex.jl

ff09c41

Update base/regex.jl

ca7b4c1

Update base/regex.jl

5f374f2

Update base/regex.jl

1bd19ff

count and findall support for Char

ec0285d

Update NEWS.md

4224af1

Update NEWS.md

b8b4546

findall and count accept Char

40b2002

Update NEWS.md

9a15719

stevengj reviewed Dec 9, 2020

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

stevengj removed the needs news A NEWS entry is required for this change label Dec 9, 2020

Update NEWS.md

7199124

Co-authored-by: Steven G. Johnson <stevenj@mit.edu>

stevengj approved these changes Dec 11, 2020

View reviewed changes

StefanKarpinski merged commit 26b73d6 into JuliaLang:master Dec 11, 2020

stevengj added a commit that referenced this pull request Dec 26, 2020

move NEWS from #38675 to the correct section [ci skip]

70e00a6

bicycle1885 mentioned this pull request Feb 15, 2021

add findall(char, str) and count(char, str) #35137

Closed

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021

move NEWS from JuliaLang#38675 to the correct section [ci skip]

e460882

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept AbstractChar in count and findall (::Char, ::String) #38675

Accept AbstractChar in count and findall (::Char, ::String) #38675

lmiq commented Dec 3, 2020

stevengj commented Dec 3, 2020

bradeneliason commented Dec 3, 2020

StefanKarpinski commented Dec 4, 2020

lmiq commented Dec 5, 2020

StefanKarpinski commented Dec 5, 2020

StefanKarpinski commented Dec 5, 2020

lmiq commented Dec 5, 2020 •

edited

Loading

StefanKarpinski commented Dec 5, 2020 •

edited

Loading

lmiq commented Dec 9, 2020

KristofferC commented Dec 9, 2020

lmiq commented Dec 9, 2020

KristofferC commented Dec 9, 2020

lmiq commented Dec 9, 2020

stevengj commented Dec 9, 2020

stevengj commented Dec 9, 2020

lmiq left a comment

Accept AbstractChar in count and findall (::Char, ::String) #38675

Accept AbstractChar in count and findall (::Char, ::String) #38675

Conversation

lmiq commented Dec 3, 2020

stevengj commented Dec 3, 2020

bradeneliason commented Dec 3, 2020

StefanKarpinski commented Dec 4, 2020

lmiq commented Dec 5, 2020

StefanKarpinski commented Dec 5, 2020

StefanKarpinski commented Dec 5, 2020

lmiq commented Dec 5, 2020 • edited Loading

StefanKarpinski commented Dec 5, 2020 • edited Loading

lmiq commented Dec 9, 2020

KristofferC commented Dec 9, 2020

lmiq commented Dec 9, 2020

KristofferC commented Dec 9, 2020

lmiq commented Dec 9, 2020

stevengj commented Dec 9, 2020

stevengj commented Dec 9, 2020

lmiq left a comment

Choose a reason for hiding this comment

lmiq commented Dec 5, 2020 •

edited

Loading

StefanKarpinski commented Dec 5, 2020 •

edited

Loading