-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-117709: Add vectorcall support for positional-only arguments of str() #117746
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-117709: Add vectorcall support for positional-only arguments of str() #117746
Conversation
Misc/NEWS.d/next/Core and Builtins/2024-04-10-22-16-18.gh-issue-117709.-_1YL0.rst
Outdated
Show resolved
Hide resolved
The speedups are as with #117725, but only for the positional-only cases. Summary of the benchmarks1:
Footnotes
|
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.
I prefer this PR over #117725 which is way more complex.
} | ||
return str; | ||
} | ||
|
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.
You may add a comment to mention that this function is a fast-path for positional-only cases.
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.
I personally prefer this one :)
Do we have enough tests for positional arguments cases?
Well, we could add something like this: diff --git a/Lib/test/test_str.py b/Lib/test/test_str.py
index b4927113db..ea37eb5d96 100644
--- a/Lib/test/test_str.py
+++ b/Lib/test/test_str.py
@@ -2651,6 +2651,24 @@ def test_check_encoding_errors(self):
proc = assert_python_failure('-X', 'dev', '-c', code)
self.assertEqual(proc.rc, 10, proc)
+ def test_str_invalid_call(self):
+ check = lambda *a, **kw: self.assertRaises(TypeError, str, *a, **kw)
+
+ # too many args
+ check(1, "", "", 1)
+
+ # no such kw arg
+ check(test=1)
+
+ # 'encoding' must be str
+ check(1, encoding=1)
+ check(1, 1)
+
+ # 'errors' must be str
+ check(1, errors=1)
+ check(1, "", errors=1)
+ check(1, 1, 1)
+
class StringModuleTest(unittest.TestCase):
def test_formatter_parser(self): UPDATE: added in 8138db4 |
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.
lgtm
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.
LGTM. I added a suggestion.
I'm not sure that arg_as_utf8() helper is strictly needed, you can keep it or inline it, it's up to you ;-)
Co-authored-by: Victor Stinner <vstinner@python.org>
Thank you so much for the reviews, Donghee and Victor! |
…y arguments (python#117746) Fall back to tp_call() for cases when arguments are passed by name. Co-authored-by: Donghee Na <donghee.na@python.org> Co-authored-by: Victor Stinner <vstinner@python.org>
Competing PR to gh-117725.
PyUnicode_Type
needs vectorcall. #117709