Case study: binary stream reading/writing: new stream methods vs support stream args for ustruct functions #10
Comments
|
Example code: Option 0: Option 1, read_into(): Option 1, read_s(): Option 2, assuming natural arg order (not compatible with the other funcs in struct): Only option 0 shows the continuity between .write() and .writebin() methods: Again, that was the whole idea of introducing them as methods. In other cases we have: which is not pretty on eyes. |
pfalcon
added a commit
that referenced
this issue
Jul 29, 2018
Comparing to pack_into(fmt, buf, offset, v1, ...), it has signature of: pack_s(fmt, stream, v1, ...), in other words accepts stream instead of buffer object, and omits offset param. See #10 for the discussion.
|
Option 0 and Option 1a/1b were implemented for actual practical comparisons: |
|
Code sizes: Option 0: x86_32: bin +496, stream.o +427 |
|
Option 1a (stream support for pack_into()): x86_32: bin +420 |
|
Option 1a (separate pack_s() function): x86_32: bin +516 |
|
So, as can be seen, an option to reuse ustruct's unpack/pack_into() gives a hundred bytes of code savings, but that's the least comfortable to use option. And introduction just the new pack_s() function leads to about same overhead as introducing 2 readbin/writebin methods. |
|
Summary: if fitting the new functionality behind the existing API was the only requirement, the unpack/pack_into() way would even give an edge on the code size. But that never was a requirement. Instead such was an introduction of new, clear and easy to use API. In that regard, excursion into fitting it back into ustruct module confirmed intuition that it would lead to poor compromise, long ago expressed in micropython/micropython#3382 . |
pfalcon
added a commit
that referenced
this issue
Aug 4, 2018
Comparing to pack_into(fmt, buf, offset, v1, ...), it has signature of: pack_s(fmt, stream, v1, ...), in other words accepts stream instead of buffer object, and omits offset param. See #10 for the discussion.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is continuation of micropython/micropython#3382 . That PR proposed adding methods like stream.readbin(fmt) and stream.writebin(fmt, val). There was criticism/suggestion to instead implement everything on the level of
ustructmodule.Option 0 (original):
Option 1: Extending existing unpack()/pack_into() functions.
Plan:
So far so good, and even nice. However, packing is worse:
offsetwould be always 0, and in general would be cumbersome/confusing (this point was raised many times in RFC py/stream: Add .readbin() and .writebin() methods. micropython/micropython#3382). Some way to deal with that would be to allow None asoffset. (We reject variants like "if stream is passed asbufferargument, then there's nooffsetargument" - these raise confusion bar too high.)Option 2: Have readbin/writebin, but as functions in the ustruct module
As another alternative suggested in micropython/micropython#3382. This would be a kind of compromise, not as convenient as methods, but avoid dealing with baggage like pack_into() offset param above. But the next question is in what order should e.g. ustruct.writebin() take args? More specifically, how natural is to have pack_into's order like
ustruct.pack_into("B", stream, None, 100). This would lead toustruct.writebin("B", stream, 100)But (surprise) maybe it's not too natural, maybe stream arg should be first?
ustruct.writebin(stream, "B", 100),ustruct.readbin(stream, "B"). But done like that, it would mean that these new functions would round pegs in ustruct's square hole.Overall, going thru description of options 1, 2 shows that the only reasoning behind them would be the desire to stuff the new functionality into straitjacket of CPython's struct module. But the whole idea behind readbin/writebin is different/opposite - not to make questionable (bad?) compromises with the old API, but add an API which many other languages have, to avoid the situation that Python (don't mix up with CPython) falls short to them.
The text was updated successfully, but these errors were encountered: