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-65002: Make note that null bytes are used to pad bytes #98635

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

slateny
Copy link
Contributor

@slateny slateny commented Oct 25, 2022

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Oct 25, 2022
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 25, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@@ -194,7 +194,7 @@ platform-dependent.
+--------+--------------------------+--------------------+----------------+------------+
| Format | C Type | Python type | Standard size | Notes |
+========+==========================+====================+================+============+
| ``x`` | pad byte | no value | | |
| ``x`` | pad byte | no value | | \(7) |
Copy link
Member

Choose a reason for hiding this comment

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

It would be best if this footnote were the first. Is (0) allowed or must footnote numbering start with 1?
If the latter, I'm NOT asking for renumbering in this situation.

A deeper question: by 'null byte', I presume the footnote means \0. Why put this in a footnote? Why not

Suggested change
| ``x`` | pad byte | no value | | \(7) |
| ``x`` | \0 pad byte | no value | | |

Copy link
Member

Choose a reason for hiding this comment

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

Alternative

Suggested change
| ``x`` | pad byte | no value | | \(7) |
| ``x`` | pad byte (\0) | no value | | |

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned on Discord, this isn't a footnote at all, its just arbitrary text—so it could be made anything at all, including 0. They should be made actual footnotes ([1]_/.. [1]), or much better auto-numbered footnotes ([#note-x]_/.. [#note-x]) that would avoid this problem and also getting out of sync and the reader doesn't have to manually scroll multiple pages to find the note (which I've had to do quite a bit on this particular table). However, I considered that out of scope of this particular PR, and better done in a followup, which is why the numbering didn't bother much (as it would be quickly fixed anyway).

I presume the clarification in the footnote is for when this is used to output rather than input a binary struct. My concern here is that since the behavior is not 1:1 on input and output (pad bytes on input are ignored, while on output they area always \0), I'm concerned this might mislead users into thinking that input bytes must be \0, rather than simply ignored. Also, it should be a literal, so \0

Incorporating @zware 's suggestion to resolve this, as well as making it a literal:

Suggested change
| ``x`` | pad byte | no value | | \(7) |
| ``x`` | pad byte | no value | | |
| | (output as ``\0``) | | | |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by input a binary struct? Like a = bytearray(b'\0Y')?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is if you call struct.unpack with x in the format on some struct-packed bytes as input, the bytes ignored by x don't have to be \0 (as the previous suggestion could imply), but rather could be anything. \0 is only what is output/packed, not nessesarily what is input/unpacked. I.e. on output/packing,

>>> struct.pack('!xxc', b'a')
b'\x00\x00a'

but those bytes can be anything on input/unpacking, not just \0, i.e.

>>> struct.unpack('!xxc', b'\0\0a')
(b'a',)
>>> struct.unpack('!xxc', b'42a')
(b'a',)
>>> struct.unpack('!xxc', b'\r\na')
(b'a',)

where as "\0 pad byte" may imply to the user that only the first might be accepted.

Comment on lines +294 to +296
(7)
For padding, ``x`` inserts null bytes.

Copy link
Member

Choose a reason for hiding this comment

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

If either change suggested above is accepted,

Suggested change
(7)
For padding, ``x`` inserts null bytes.

@ambv
Copy link
Contributor

ambv commented Oct 28, 2022

I agree with CAM that it would be good to move to auto-numbered footnotes. This, however, is not the point of the PR in question. So, let's just merge that first. We can fix the pseudo-footnotes in a dedicated PR.

@ambv ambv merged commit 8cd21c2 into python:main Oct 28, 2022
@ambv ambv added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Oct 28, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-98803 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 28, 2022
@bedevere-bot
Copy link

GH-98804 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 28, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 28, 2022
…onGH-98635)

(cherry picked from commit 8cd21c2)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 28, 2022
…onGH-98635)

(cherry picked from commit 8cd21c2)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Oct 28, 2022
@slateny slateny deleted the s/65002 branch October 29, 2022 06:51
@slateny
Copy link
Contributor Author

slateny commented Oct 29, 2022

There was some discussion on whether to move the footnote into the table itself above, but now that this is merged perhaps we just leave that for another time if wanted.

miss-islington added a commit that referenced this pull request Nov 3, 2022
(cherry picked from commit 8cd21c2)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Nov 3, 2022
(cherry picked from commit 8cd21c2)

Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants