test: Remove struct.pack from almost all places #29401
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
Concept ACK. Is this ready for testing? |
ceb63b3 to
830e908
Compare
830e908 to
fa855d8
Compare
* Add () around some int values * Remove b-prefix from strings This is needed for the scripted diff to work.
This is done in prepration for the scripted diff, which can not deal with those lines.
-BEGIN VERIFY SCRIPT- sed -i --regexp-extended 's!struct.pack\(.<?B., (.*)\)!\1.to_bytes(1, "little")!g' $( git grep -l struct.pack ) sed -i --regexp-extended 's!struct.pack\(.<I., (.*)\)!\1.to_bytes(4, "little")!g' $( git grep -l struct.pack ) sed -i --regexp-extended 's!struct.pack\(.<H., (.*)\)!\1.to_bytes(2, "little")!g' $( git grep -l struct.pack ) sed -i --regexp-extended 's!struct.pack\(.<i., (.*)\)!\1.to_bytes(4, "little", signed=True)!g' $( git grep -l struct.pack ) sed -i --regexp-extended 's!struct.pack\(.<q., (.*)\)!\1.to_bytes(8, "little", signed=True)!g' $( git grep -l struct.pack ) sed -i --regexp-extended 's!struct.pack\(.>H., (.*)\)!\1.to_bytes(2, "big")!g' $( git grep -l struct.pack ) -END VERIFY SCRIPT-
fa855d8 to
fa52e13
Compare
There was a problem hiding this comment.
tACK fa52e13
Make successful, so are all the functional tests.
Overall I agree with the intent to get rid of struct.pack usage, which requires looking up the documentation most of the time and might be prone to errors. Using the alternate approach of to_bytes is more self-documenting and preferable to me.
|
Concept ACK |
| @@ -117,11 +116,11 @@ def ser_compact_size(l): | |||
| if l < 253: | |||
| r = l.to_bytes(1, "little") | |||
| elif l < 0x10000: | |||
| r = struct.pack("<BH", 253, l) | |||
| r = (253).to_bytes(1, "little") + l.to_bytes(2, "little") | |||
There was a problem hiding this comment.
nit: for serializing single bytes I personally prefer bytes([value]) over the .to_bytes method, e.g.
| r = (253).to_bytes(1, "little") + l.to_bytes(2, "little") | |
| r = bytes([253]) + l.to_bytes(2, "little") |
(but no blocker, feel free to ignore)
There was a problem hiding this comment.
I agree, but for consistency with the other def ser_compact_size and the other code, I'll leave as-is for now.
|
ACK fa52e13 |
struct.packhas many issues:This lead to many test bugs, which weren't hit, which is fine, but still confusing. Ref: #29400, #29399, #29363, fa3886b, ...
Fix all issues by using the built-in
inthelpersto_bytesvia a scripted diff.Review notes:
struct.packandint.to_bytesthe error behavior is the same, although the error messages are not identical.struct.packremain. One for float serialization in a C++ code comment, and one for native serialization.