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

gh-94808: Cover str.rsplit for UCS1, UCS2 or UCS4 #98228

Merged
merged 3 commits into from
Oct 15, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 12, 2022

This code was not covered:
Снимок экрана 2022-10-13 в 1 46 36

Now, it is!

@vstinner I got curious and looked into other asciilib_* calls. The setup is the same as in #98025 (comment)
For example, asciilib_rsplit_whitespace also does not give any significant gain:
./env/bin/python.exe -m pyperf timeit -v '"a b ca b d".rsplit()'

// With asciilib call:
// Mean +- std dev: 134 ns +- 4 ns
// Mean +- std dev: 143 ns +- 5 ns

// Without it:
// Mean +- std dev: 137 ns +- 2 ns
// Mean +- std dev: 140 ns +- 7 ns

I will open a new issue about where we can discuss this!

@sobolevn
Copy link
Member Author

Issue about asciilib performance: #98229

@@ -445,10 +445,10 @@ def test_split(self):
def test_rsplit(self):
string_tests.CommonTest.test_rsplit(self)
# test mixed kinds
for left, right in ('ba', '\u0101\u0100', '\U00010301\U00010300'):
for left, right in ('ba', 'юё', '\u0101\u0100', '\U00010301\U00010300'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this cover all of the UCS-1, UCS-2, and UCS-4 branches? Might be worth commenting on which versions are covered by which characters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places don't have comments about it, so I didn't bother adding one as well.
I believe that this does cover all unicode kinds: ba', 'юё' are UCS-1, \u0101\u0100 is UCS-2 and \U00010301\U00010300 is UCS-4

@JelleZijlstra JelleZijlstra merged commit b7dd2ca into python:main Oct 15, 2022
@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-98291 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 15, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 15, 2022
JelleZijlstra pushed a commit that referenced this pull request Oct 20, 2022
…#98291)

gh-94808: Cover `str.rsplit` for UCS1, UCS2 or UCS4 (GH-98228)
(cherry picked from commit b7dd2ca)

Co-authored-by: Nikita Sobolev <[email protected]>
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.

5 participants