GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-94808: Cover `str.rsplit` for UCS1, UCS2 or UCS4 by sobolevn · Pull Request #98228 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 12, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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!

  • Issue: Metabug: Improving C-level coverage #94808

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn
pythongh-94808: Cover str.rsplit for UCS1, UCS2 or UCS4
523e5bb
@bedevere-bot bedevere-bot added the awaiting review label Oct 12, 2022
@sobolevn sobolevn added the skip news label Oct 12, 2022
sobolevn added 2 commits October 13, 2022 01:56
@sobolevn
Merge branch 'main' into cover-str-rsplit
a3a039c
@sobolevn
Add more cases
06f86c9
@sobolevn sobolevn mentioned this pull request Oct 12, 2022
Do we still need asciilib for better performance? #98229
Open
@sobolevn

sobolevn commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Issue about asciilib performance: #98229
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mdboom mdboom mentioned this pull request Oct 13, 2022
Metabug: Improving C-level coverage #94808
Open
@mdboom mdboom added the needs backport to 3.11 only security fixes label Oct 13, 2022
JelleZijlstra
JelleZijlstra reviewed Oct 15, 2022
View reviewed changes
Comment thread Lib/test/test_unicode.py
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'):

@JelleZijlstra JelleZijlstra Oct 15, 2022

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sobolevn sobolevn Oct 15, 2022

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@JelleZijlstra
JelleZijlstra merged commit b7dd2ca into python:main Oct 15, 2022
@bedevere-bot bedevere-bot removed the awaiting review label Oct 15, 2022
@miss-islington

miss-islington commented Oct 15, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Oct 15, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@sobolevn @miss-islington
pythongh-94808: Cover str.rsplit for UCS1, UCS2 or UCS4 (pythonGH-9…
7be0bdd 
…8228)

(cherry picked from commit b7dd2ca)

Co-authored-by: Nikita Sobolev <[email protected]>
JelleZijlstra pushed a commit that referenced this pull request Oct 20, 2022
@miss-islington @sobolevn
[3.11] gh-94808: Cover str.rsplit for UCS1, UCS2 or UCS4 (GH-98228) (…
df4aaff 
…#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

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@sobolevn @miss-islington @bedevere-bot @JelleZijlstra @mdboom
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.