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

Re-implement String.get_slice_count as count() + 1, which has the same logic. #101030

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ivorforce
Copy link
Contributor

I think the new version is more readable :)
There may be a tiny speed difference because the _count implementation supports p_from and p_to, but it's unlikely to be even measurable.

Both implementations skip over the full occurrence, i.e. "fff".count("ff") = 1.

@Ivorforce Ivorforce requested a review from a team as a code owner January 2, 2025 16:49
@AThousandShips AThousandShips added this to the 4.x milestone Jan 2, 2025
@AThousandShips
Copy link
Member

One problem is that _count unconditionally copies the string being searched in

I'd say this should be benchmarked to compare as I'm not convinced it will be as performant

@Ivorforce
Copy link
Contributor Author

One problem is that _count unconditionally copies the string being searched in

I'd say this should be benchmarked to compare as I'm not convinced it will be as performant

The string will only be copied if p_from and p_to are > 0, which is not the case for get_slice_count as they default to 0.
There used to be more copies, but I have fixed them recently in #100294.

@AThousandShips
Copy link
Member

No it copies in all cases:

godot/core/string/ustring.cpp

Lines 3703 to 3711 in 2582793

if (p_from >= 0 && p_to >= 0) {
if (p_to == 0) {
p_to = len;
} else if (p_from >= p_to) {
return 0;
}
if (p_from == 0 && p_to == len) {
str = String();
str.copy_from_unchecked(&get_data()[0], len);

The only time it doesn't copy is when it returns 0 because of the range

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Jan 2, 2025

No it copies in all cases

You're right, it totally does. I have misread the if condition.
I have quickly opened #101033 to address this. In the PR it will use a CoW copy instead of a full copy, which isn't free but it's likely negligible in most cases.

@Ivorforce
Copy link
Contributor Author

I just noticed this PR does change behavior on 0-size strings. See the related discussions on #82484.

There is an open PR to address this: #82620

It would be incorrect to merge this PR before the desired behavior is agreed upon, so I'm going to mark this PR as draft until we get there.

@Ivorforce Ivorforce marked this pull request as draft January 9, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants