-
Notifications
You must be signed in to change notification settings - Fork 64
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
Don't use argument unpacking on strings #57
Conversation
Ouch! In [1]: import struct
In [2]: data = b" "*100000
In [3]: % timeit struct.pack('100000s',data) # with this patch
The slowest run took 4.30 times longer than the fastest. This could mean that an intermediate result is being cached
100000 loops, best of 3: 15.3 µs per loop
In [4]: % timeit struct.pack('100000B',*data) # without it
100 loops, best of 3: 7.48 ms per loop |
And at the extreme: In[1]: import struct
In[2]: data = b" " * 1000000000
In[3]: %timeit struct.pack('s', data)
The slowest run took 7.66 times longer than the fastest. This could mean that an intermediate result is being cached
1000000 loops, best of 3: 487 ns per loop
In[4]: %timeit struct.pack('B', *data)
MemoryError |
@@ -166,8 +166,8 @@ def test_default_value(): | |||
assert '[0,0,0,0]' == default_value(msg_context, t+'[4]', 'std_msgs') | |||
assert '[0]' == default_value(msg_context, t+'[1]', 'roslib') | |||
|
|||
assert "chr(0)*1" == default_value(msg_context, 'uint8[1]', 'roslib') | |||
assert "chr(0)*4" == default_value(msg_context, 'uint8[4]', 'roslib') | |||
assert b'\0' == eval(default_value(msg_context, 'uint8[1]', 'roslib') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid syntax: lacks trailing )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I had no idea why the marker was pointing at the start of the line in the traceback
I think it would make a lot more sense for every assertion in this test of be of the form |
These tests now fail because the python output of the generator has changed. At some point I'll update the expected output files to match the new output |
`struct.pack('5s', b'abcde')` works just fine for me on 3.5.0
And so `chr` is also not safe. `b'\0' works fine in both versions.
Thank you for the patch. Merged to kinetic-devel in #63. |
struct.pack('5s', b'abcde')
works just fine for me on 3.5.0. Argument unpacking a string is going to be a big performance hit for long strings too