Merge from mattsta's PR collection #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixed the issue where +-inf of NaNs would be encoded as integers. Added a header file which isolates inclusions and conditionally defines macros for inspecting float-type to integral-type conversions in a portable fashion.
Now pack can receive any number of arguments returning a stream of the serialized data (on 0 objects, returns the empty string). unpack deserialized any stream of objects, returning the result of the whole stream deserialization (this, the number of return values of unpack is equal to the number of objects unpacked). If given the empty string, returns nothing. I also removed all warnings and made the code C++ compatible. [untested at this point]
@moteus suggested this change in moteus@7f64c4f That patch is a bit of a mess (it's a merge of somebody else's work squashed with about four other changes too), so I re-implemented the safe functionality in a slightly cleaner way (using Lua C closures with upvalues). Now we have a cmsgpack.safe module that won't throw errors, but rather return them as clean (nil, err) return values. Minor API change: calling pack() with no arguments now throws an error instead of returning an empty string (because nothing was encoded, we shouldn't return anything resembling usable data). Also: now we prefer luaL_argerror for cleaner errors caused by invalid arguments and also luaL_error for easier msg-and-return-error functionality. Double also: now we prefer to call return on error functions so it's clear to us the Lua error functions do long jumps and never return control to the original function from then onward.
This was a complex way of saying "make the top of stack the current top of the stack" and caused no side effects.
We're using CMake here because building a Makefile takes
longer and won't automatically find your system-wide Lua
for including against.
You can run the tests with:
mkdir build
cd build
cmake ..
lua ../test.lua
As of this commit all tests pass:
TEST PASSED: 207
TEST FAILED: 0
TEST SKIPPED: 0
This commit adds many new tests and test checking methods for existing
functionality and the new streaming/multi-pack/multi-return functionality.
Thanks to @moteus for testing suggestions which I've included.
Sadly, I couldn't include the changes directly because they were too intermixed
with the rest of a 250 line commit[1].
[1]: moteus@7f64c4f
Lua 5.3 adds an actual Integer type, which by default
will be a 64-bit long long. Currently, Lua {5.1,5.2}
only has a Number type which by default is a 64-bit
double.
Note: Lua can be custom compiled to re-define Integer
to be 32 bits (as well as Number to be just a float), but
that should be rare and the interal cmsgpack conversion
routes store integers in the most compact representation
discernable.
This also uses the existing lua_tointeger function instead of
casting a lua_Number to an int64_t directly.
Also remove empty end-of-line spaces from a few places.
Inspired by antirez#10, I added two new functions allowing you to limit multiple return from unpack. If you don't trust your msgpack input or want to manage flow control of returned object generation yourself, you can now limit the objects returned from unpack (but not with unpack() itself because it already returns an arbitrary number of objects). You can now unpack_one(msgpack) to get (next_offset, obj) returned; then call unpack_one(msgpack, next_offset) to get another offset and one more object> You can now also unpack_limit(msgpack, N) to get (next_offset, obj1, obj2, ..., objN) returned; then call unpack_limit(msgpack, K, next_offset) to get another offset and K more objects. This also refactors the copy/paste command addition in module bring-up. Tests added for new commands and all tests pass: TEST PASSED: 225 TEST FAILED: 0 TEST SKIPPED: 0 Closes antirez#10
We don't really need malloc here since we use it and collect it all in the same spot without ever needing to resize anything.
We need a test to pack something larger than the default allocation buffer.
This fixes Lua 5.3 compatability and works fine on Lua 5.1 too. Lua 5.3 has an actual 64-bit Integer type, so we need to use _pushinteger instead of _pushnumber so our unpacked integers don't convert to floating point values. For older versions, lua_pushinteger casts input to the default numeric type and behavior is unchanged. Lua 5.3 also has an unsigned integer type, so I added a compatibility macro to allow us to properly use lua_pushunsigned on 5.3+ with a fallback to the regular lua_pushinteger on other versions.
Lua 5.3 doesn't iterate tables in any specific order, so the resulting msgpack may vary when using associative tables with more than one element. I've only seen two variations on the the regression test output so far, so tracking those are still simple. If they get wildly out of sync and start generating new msgpack on every run, we may want to drop the exact output verification check. The recursive regression test now has a circular test too, so even if the output doesn't match, we can verify the unpacked result matches the input result (to a fixed depth). As of this commit, all tests pass on Lua 5.1 and Lua 5.3: TEST PASSED: 226 TEST FAILED: 0 TEST SKIPPED: 0
We can reuse the same malloc'd buffer for encoding multiple arguments. Just tell the buffer all current contents is now free space and reset the current offset back to zero. We now only malloc once per pack() call instead of argc times per call, giving us better performance with large argument counts. All 226 tests pass.
cmsgpack is a much better Lua citizen now because we use the memory allocator assigned to our lua_State instead of grabbing memory from the system ourselves. Typically the memory allocator is just the system's realloc, but some use cases involve providing custom allocation routines (for accounting or performance or limiting memory). This closes antirez#20 too because those commits were just trying to remove the previous direct allocation behavior. All tests pass under Lua 5.1 and Lua 5.3-work2.
If more than ~20 objects were being returned at once, Lua would segfault because the default stack size is 20 and nobody was resizing the stack. Now we can return up to ~8,000 objects at once before erroring out the function properly instead of segfaulting. Also added test for segfault. All tests currently pass.
Closes antirez#31
'unpack' is only on 5.1, in 5.2 it got moved to 'table.unpack' This line gives us the proper 'unpack' based on what's in our global namespace.
Changes: - Improve table vs. array detection - Improve packing +-inf - Add multiple pack/unpack support - Add cmsgpack.safe module variant - Add local build infrastructure for easier testing - Add user-controlled unpack support limiting returned objects - Add Lua 5.3 compatibility - Remove an unnecessary malloc - Use Lua memory allocator instead of malloc for buffer creation Issues involved: - closes antirez#16 - allow multi pack/unpack by default - closes antirez#10 - unpack one/limit API added - closes antirez#13 and closes antirez#20 - use Lua allocator - closes antirez#15 - (included in antirez#16) - ignores antirez#22 because it's confusing - closes antirez#23 - fixed elsewhere - closes antirez#26 - extracted some useful parts from a difficult commit - closes antirez#28 - we started tagging versions again recently - closes antirez#27 - that failure case works for me now - closes antirez#31 - fix comment typos I merged commits with original author information where possible, but each commit required manual cleanup of one or more of: formatting fixes (no tabs, please), commit message fixes (more details please), extracting contents from a single 300 line commit with 5 different logical changes merged together, and general correctness checking after merging with newer code. As of this commit, all tests pass on Lua 5.1.5 and Lua 5.3-work2.
The 0xFFFFFFFF test can potentially fail on 32 bit systems if our internal conversions are casting to wrong Lua types. lua_Integer != lua_Number
cheyilin
added a commit
that referenced
this pull request
Oct 27, 2014
Merge from mattsta's PR collection
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original PR: antirez#30