Skip to content

gh-143672: convert the struct module to AC - #143673

Closed
skirpichev wants to merge 9 commits into
python:mainfrom
skirpichev:struct-AC/143672
Closed

gh-143672: convert the struct module to AC#143673
skirpichev wants to merge 9 commits into
python:mainfrom
skirpichev:struct-AC/143672

Conversation

@skirpichev

@skirpichevskirpichev commented Jan 11, 2026

Copy link
Copy Markdown
Member

Comment threadModules/_struct.c

See help(struct) for more on format strings.
Return a new Struct object which writes and reads binary data according
to the format string. See help(struct) for more on format strings.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is the reason for newline -> double space change? I was not aware that this is required.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In some docstrings "See help(struct) for more on format strings." was separated to a new paragraph. In some - no. Looks as a waste of space, especially in help(struct) output.

Comment threadModules/_struct.c Outdated
/
*args: array

Pack args to the writtable buffer according to the format self.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Packargstothewrittablebufferaccordingtotheformatself.
Packargstothewrittablebufferaccordingtothestruct's format string.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Too long. Beware @permit_long_summary.

Comment threadModules/_struct.c Outdated
strings.");
*args: array

Return a bytes object with args, packed according the format self.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Returnabytesobjectwithargs, packedaccordingtheformatself.
Returnabytesobjectwithargs, packedaccordingthestruct's format string.

@skirpichev

Copy link
Copy Markdown
MemberAuthor

BTW, I'm not attached to docstrings.

IMO, they aren't good and I tried to improve that. But I can just revert that stuff and keep old doctsrings almost literally (summary line is required, though).

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I have not noticed this PR and created an alternative PR #143857.

There are some differences between two PRs, and I had reasons for all changes. We can now gradually convert your PR into my PR by applying one change at a time or just take my PR.

See the difference between the PRs: https://github.com/skirpichev/cpython/pull/14/changes .

Comment threadModules/_struct.c
Struct.pack_into as s_pack_into

buffer: Py_buffer
offset: Py_ssize_t

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a subtle behavior difference here (IndexError vs OverflowError). I leave this for other issue.

Comment threadModules/_struct.c Outdated
buffer: Py_buffer
offset: Py_ssize_t
/
*args: array

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prever to use more specific name values.

Comment threadModules/_struct.c Outdated
/*[clinic input]
Struct.pack_into as s_pack_into

buffer: Py_buffer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The buffer must be writeable.

Comment threadModules/_struct.c
offset. Note that the offset is a required argument. See\n\
help(struct) for more on format strings.");
/*[clinic input]
Struct.pack_into as s_pack_into

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to rename it.

Sign up for freeto 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.

3 participants

@skirpichev@serhiy-storchaka@sobolevn