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

bpo-28307: Optimize C-style formatting of numbers #26160

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 16, 2021

Support format codes %d, %o, %x, %f, %e, %g, etc.

https://bugs.python.org/issue28307

Support format codes %d, %o, %x, %f, %e, %g, etc.
* ``ord('r')``: call :func:`repr` before formatting (``!r``)
* ``ord('a')``: call :func:`ascii` before formatting (``!a``)
* ``ord('d')``: convert to :class:`int` with truncating
(for internal use only)
Copy link
Member

Choose a reason for hiding this comment

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

What does "(for internal use only)" mean? What restricts this to being used one way or another?

I think we need different text here. As f-strings don't support !d, !i, and !f conversions directly we should just say that rather than document values that may be encountered but imply that people are not supposed to use them.

They're going to use them in their own ast ndoes and in their own bytecode generation (for people who do that sort of thing...) no matter what we say.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed (for internal use only).

def test_format(self):
flags = '-+ #0'
testcases = [
*product(('', '1234',), 'sra'),
Copy link
Member

Choose a reason for hiding this comment

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

Test case(s) that include other % formatting directives within the value would be good.

We want to make sure injection attacks don't exist if this happened to get implemented in the wrong serialized actions manner.

untrusted = '%% %s'
value = 'foo %s bar' % untrusted

results in value == 'foo %% %s bar' rather than raising an exception about tuple size or converting %% into %.

This might make a good test of its own rather than shoehorning it into this one given some complexities of where % directives that'll be peepholed can appear in strings and in combination with others that may or may not be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added more tests. Caught and fixed conversion of non-numbers to int and float ('%d' % '123' did not raise exception before).

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 24, 2021
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 8, 2022
@iritkatriel iritkatriel added the performance Performance or resource usage label Aug 31, 2022
@skirpichev
Copy link
Member

This now has merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants