Skip to content

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Aug 4, 2025

Fix #17068

Adds signed integer endianness support to pack()/unpack() functions. There are 2 new format codes (from the original issue): m/y for signed 2-byte (little/big endian), M/Y for signed 4-byte (little/big endian), and p/j for signed 8-byte (little/big endian). It follows the existing pattern where lowercase matches uppercase endianness.

I also moved pack tests in a dedicated folder.

Copy link
Member

@NattyNarwhal NattyNarwhal left a comment

Choose a reason for hiding this comment

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

Passes on a big endian system

@alexandre-daubois
Copy link
Member Author

Friendly ping @php/release-managers, would you prefer to wait or are you comfortable with this change?

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

RM approval, technical review not performed

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Technically this looks okay.
I didn't understand the reasoning behind picking the letters for the format, and the original issue description didn't help me understand it either. The original issue comment talks about "to match the other formats" but I didn't understand this. Can you please explain?

@alexandre-daubois
Copy link
Member Author

I followed the current convention where lowercase is little endian, and uppercase is big endian.

About the letters themselves, I'm not sure there's a technical justification other than "these letters are not already used in pack/unpack". Maybe I miss something and the letters may actually have a deeper meaning?

@nielsdos
Copy link
Member

I followed the current convention where lowercase is little endian, and uppercase is big endian.

You didn't follow this.
Also this doesn't seem to be the convention. It seems mostly true though.
Thing is I want to be sure about the choice as we'd be stuck with this forever.

@alexandre-daubois
Copy link
Member Author

Indeed, I've got it all mixed up. It's no good as it is.
Would this logic work? If so, I'll update the PR accordingly.

@nielsdos
Copy link
Member

Indeed, I've got it all mixed up. It's no good as it is. Would this logic work? If so, I'll update the PR accordingly.

Possibly, but honestly the more I look at the table the less the naming scheme makes sense, I just don't get it. Someone else will have to judge what the right letters are...

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

This should match Perl: https://perldoc.perl.org/functions/pack

>   sSiIlLqQ   Force big-endian byte-order on the type.
    jJfFdDpP   (The "big end" touches the construct.)

<   sSiIlLqQ   Force little-endian byte-order on the type.
    jJfFdDpP   (The "little end" touches the construct.)

@alexandre-daubois
Copy link
Member Author

Hmmm... PERL uses < and > modifiers for endianness, which isn't compatible with the current implementation of pack/unpack that only accept one char. Thus we cannot match PERL's pack exactly. I'm not sure how we should proceed here apart from taking arbitrary letters?

@cmb69
Copy link
Member

cmb69 commented Sep 15, 2025

In my opinion, it's better to pursue the RFC process here, instead of adding some arbitrary characters which might later interfere with further additions.

@alexandre-daubois
Copy link
Member Author

Looks like the best thing to do here. Let's pause this PR for now

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.

Add pack/unpack support for signed integers with specific endianness
7 participants