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
[mypyc] Faster bytes formatting #10989
Conversation
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.
Looks good, thanks! Left some minor comments. Optimizing non-ascii literals can be left for a follow-up issue if it's non-trivial (they just need to work correctly).
# https://www.python.org/dev/peps/pep-0461/ | ||
def test_bytes_formatting() -> None: | ||
val = 10 | ||
assert b"%x" % val == b'a' |
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.
Test producing an empty bytes object (e.g. b"%b" % bytes()
and b"" % ()
).
Add test for __bytes__
conversion (of create an issue if it doesn't work yet).
@@ -64,3 +64,11 @@ | |||
return_type=bytes_rprimitive, | |||
c_function_name='CPyBytes_Join', | |||
error_kind=ERR_MAGIC) | |||
|
|||
bytes_build_op = custom_op( |
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.
Add comment.
"""Merge the list of literals and the list of substitutions | ||
alternatively using 'bytes_build_op'. | ||
|
||
The literals should only contains ascii literal objects. |
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.
Do these need to be ascii? Is it sufficient for them to be arbitrary bytes literals?
|
||
for a, b in zip(literals, substitutions): | ||
if a: | ||
result_list.append(builder.load_bytes(a.encode('ascii'))) |
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.
If you'd use latin1
as the encoding, could we support non-ascii bytes values?
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.
The literals inside BytesExpr
is guaranteed to be ascii literals, otherwise it won't pass the type-checker.
I checked that this makes the |
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.
Thanks for the updates! Looks good.
Description
convert_expr_to_bytes
that convert values to bytes.CPyBytes_Build
andjoin_formatted_bytes
.Test Plan
Several run tests and an irbuild test for
CPyBytes_Build