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-95504: Fix sign placement in PyUnicode_FromFormat #95505

Closed
wants to merge 8 commits into from

Conversation

philg314
Copy link
Contributor

@philg314 philg314 commented Jul 31, 2022

gh-95504: Fix sign placement in PyUnicode_FromFormat

The sign doesn't stick to the number anymore:
PyUnicode_FromFormat("%05d", -123); results in "-0123" instead of "0-123",
PyUnicode_FromFormat("%.5d", -123); results in "-00123" instead of "0-123".

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jul 31, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

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

LGTM.
But this is bit sensitive to backport because Python 3.11 is right before RC.

@philg314
Copy link
Contributor Author

philg314 commented Aug 1, 2022

I emailed core-mentorship about this on Friday night but didn't wait to hear back before submitting this PR.

@encukou is in the process of splitting _testcapimodule.c up into more manageable pieces (#93649) and has asked me to move Unicode related tests into a separate file. I'll convert this PR to a draft while working on that and convert it back once that's done.

@philg314 philg314 marked this pull request as draft August 1, 2022 15:33
@philg314 philg314 marked this pull request as ready for review August 2, 2022 03:28
@philg314 philg314 requested a review from a team as a code owner August 2, 2022 03:28
@philg314
Copy link
Contributor Author

philg314 commented Aug 6, 2022

@encukou this is ready for review.

@serhiy-storchaka serhiy-storchaka requested review from serhiy-storchaka and removed request for a team August 7, 2022 08:50
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM in general, but I would wait for testcapi refactoring.

Misc/ACKS Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
Comment on lines +2507 to +2510
if (negative && !zeropad) {
if (_PyUnicodeWriter_WriteChar(writer, '-') == -1)
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

It can be simpler. Instead of writing minus explicitly, you can write it as a part of the buffer.

You will get the same result if remove this if and pass &buffer[negative && zeropad] instead of &buffer[negative] to _PyUnicodeWriter_WriteASCIIString().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work with precision: %.5d with -123 would be 00-123.

@encukou
Copy link
Member

encukou commented Aug 8, 2022

Sorry for making you do more work, but it works best if the test refactoring is done first, in a separate pull request, so that:

  • the change PR shows what changed in the tests
  • if something goes wrong, the change can be reverted without the refactoring
  • the refactoring can be applied to older unfixed versions, if necessary (e.g. wep only need to backport a subset of changes)

Do you want to split this up? I can also do it, and it wouldn't be too much extra work in addition to a review, but I don't want to “steal your contribution”.

@philg314
Copy link
Contributor Author

philg314 commented Aug 8, 2022

Sorry for making you do more work, but it works best if the test refactoring is done first, in a separate pull request, so that:

  • the change PR shows what changed in the tests
  • something goes wrong, the change can be reverted without the refactoring
  • refactoring can be applied to older unfixed versions, if necessary (e.g. wep only need to backport a subset of changes)

No problem, makes sense.

Do you want to split this up? I can also do it, and it wouldn't be too much extra work in addition to a review, but I don't want to “steal your contribution”.

Sure, and for future reference I'll never have a problem with anything like that. Please ping me when the refactor is merged.

@encukou
Copy link
Member

encukou commented Aug 10, 2022

@philg314,
Tests were merged in #95819.
The behavior change is in #95848, could you check everything is in order?

@encukou
Copy link
Member

encukou commented Aug 10, 2022

The changes were merged in #95819 and #95848.

Thanks for the contribution, @philg314!

@encukou encukou closed this Aug 10, 2022
@philg314
Copy link
Contributor Author

Thanks for the reviews @methane and @serhiy-storchaka and thanks for merging @encukou!

@philg314 philg314 deleted the unicode-from-format-negative branch August 10, 2022 14:28
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