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-39689: Do not use native packing for format "?" with standard size #18969

Merged
merged 1 commit into from Mar 24, 2020

Conversation

@skrah
Copy link
Member

skrah commented Mar 12, 2020

@skrah

This comment has been minimized.

Copy link
Member Author

skrah commented Mar 12, 2020

This implements the current documentation somewhat pedantically:

The '?' conversion code corresponds to the _Bool type defined by C99. If this type is not available, it is simulated using a char. In standard mode, it is always represented by one byte.

We always have C99 now, so native mode always uses _Bool. When compiling with
-fsanitize=bool, this should (and does) fail:

>>> from struct import *
>>> unpack("?", b"\x02")
/home/stefan/cpython/Modules/_struct.c:487:12: runtime error: load of value 2, which is not a valid value for type '_Bool'
(False,)

This, however, should pass (and does after this patch):

>>> unpack("<?", b"\x02")
(True,)
Copy link
Member

encukou left a comment

The docs currently say:

For the '?' format character, the return value is either True or False. When packing, the truth value of the argument object is used. Either 0 or 1 in the native or standard bool representation will be packed, and any non-zero value will be True when unpacking.

So, if we do want change that and expose UB to Python, the docs need to change. IMO, this will need a news entry as well.

The docs also currently talk about the characters @=<>! only affecting byte order, size, and alignment. Will that need to be expanded?

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Mar 17, 2020

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@miss-islington miss-islington merged commit 472fc84 into python:master Mar 24, 2020
8 checks passed
8 checks passed
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200312.39 succeeded
Details
bedevere/issue-number Issue number 39689 found
Details
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link
Contributor

miss-islington commented Mar 25, 2020

Thanks @skrah for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Copy link
Contributor

miss-islington commented Mar 25, 2020

Thanks @skrah for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Mar 25, 2020

GH-19154 is a backport of this pull request to the 3.7 branch.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Mar 25, 2020

GH-19155 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Mar 25, 2020
pythonGH-18969)

(cherry picked from commit 472fc84)

Co-authored-by: Stefan Krah <skrah@bytereef.org>
miss-islington added a commit to miss-islington/cpython that referenced this pull request Mar 25, 2020
pythonGH-18969)

(cherry picked from commit 472fc84)

Co-authored-by: Stefan Krah <skrah@bytereef.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.