-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added MessagePackEncoder/Decoder for Codable support #61
base: master
Are you sure you want to change the base?
Conversation
|
||
fileprivate func unbox(_ value: MessagePackValue, as type: UInt8.Type) throws -> UInt8? { | ||
switch options.numberDecodingStrategy { | ||
case .noTypeConversion: return value.unsignedIntegerValue.map { UInt8($0) } |
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.
This type cast will cause an EXC_BAD_INSTRUCTION if the underlying integer is larger than UInt8.max. The same applies for every other implementation of unbox
that casts to integers smaller than Int64 (including Int which should not be assumed to be 64-bits).
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.
Right, i'll fix this and add test cases for this
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.
In general, this implementation looks pretty much identical to JSONEncoder.swift. My only concern is that it is so similar that I wonder if it may be subject licensing requirements found in https://swift.org/LICENSE.txt. That is, could it be considered a derivative work of JSONEncoder.swift?
Thats a valid point, I think this is definitely considered as a derivative work of JSONEncoder.swift. A rewrite will be necessary to resolve this and it might take awhile. How is your implementation coming along? |
@andrew-eng I'm not a lawyer of course. And perhaps there's case to be made that the protocol-based design of Codable basically requires that the code look pretty close to what JSONEncoder and PlistEncoder. So I'm not sure how much a rewrite will remedy the situation. But I think all you need to do is ensure that you follow the steps outlined in Redistribution of https://swift.org/LICENSE.txt and you should be fine. As for my implementation, I've gone to a good deal of effort to simplify when possible. For example, because the MessagePack serializer is pure swift, there's no need to convert objects to or from anything deriving
|
Hello, are there still plans to merge this PR? Sound's very interesting! |
there are swift.org mailing lists where you could ask for guidance https://lists.swift.org/mailman/listinfo |
@rustle @mgadda I've posted this topic on the Swift forums. Let's see what people say. |
Hey guys, sorry for the late reply, recently I decided to write my own MessagePack parser because i encounter some performance issue when parsing large data. I also rewrote the Encoder/Decoder from scratch because the code in the above PR is indeed monstrous. I am planning to port over the new Encoder/Decoder and submit a new PR in a few days time. Hopefully the new PR will be easier to manage and merge into this project. |
@andrew-eng Sounds good. Would love to see the Encoder/Decoder stuff you're working on. |
I would also love to see your implementation @andrew-eng! How did you manage to shrink the code? |
@andrew-eng I also ended up abandoning this pure swift implementation because it was far too slow for my needs. I ended up using the C-based https://github.com/camgunz/cmp which was many orders faster and integrates easily into an existing swift project. I did write some wrapping code which may or may not be useful for anyone wishing to use cmp. |
hey guys, I have submitted PR #66 containing a simplified Encoder/Decoder. Still haven't figured out why the tests ran successfully on the Linux machine but not on Mac. Will look at it over the next few days. @cherrywoods, i ended up not implementing several features that are not commonly used, turns out that simplified the code a lot. @mgadda nice find. I'll do a performance test against that. I wrote mine in pure swift while avoiding expensive operations. Curious to see how swift fare against good old C. |
Hi,
I am using MessagePack pod in my Swift project and it is great. However, currently it is quite painful to manually encode and decode my models.
I have written an implementation of MessagePackEncoder that heavily references JSONEncoder.swift from the swift foundation library. The structure and logic is almost identical to that of JSONEncoder.swift. However, it turns out that implementing a Encoder/Decoder is non-trivial and the logic is rather involved.
Therefore, I am not sure if incorporating this code into your project is the the right way forward as your library is very light weight.
Nevertheless, I'll submit a PR to further discuss this issue.