Skip to content
This repository has been archived by the owner on Sep 3, 2024. It is now read-only.

Use GenericDerive for Binary instance on Event and related types #9

Closed
wants to merge 1 commit into from

Conversation

izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 23, 2015

I am using it to support HaLVM, finding that the original serialization is written by hand and not complete for all possible data. I rewrite it with Generics and help it will helps.

@qnikst
Copy link
Contributor

qnikst commented May 27, 2015

@izgzhen, thanks for your PR and sorry for the late response!

But please can you elaborate what is an example of data it's not complete for? Currently we have decode . encode == id for universe of Reliability constructors, i.e.:

 *Network.Transport Data.Binary> let lst = [ ReliableOrdered, ReliableUnordered, Unreliable ]
*Network.Transport Data.Binary> map (decode . encode) lst == lst
True

This means that for any possible Reliability serialization will complete. However, if we will decode something that is not Reliability or decode reliability from newer version on n-t (for example if UnreliableOrdered whatever it be will be added) this function will fail, and this is expected behaviour from my point of view. Example:

*Network.Transport Data.Binary Data.Binary.Put> decode $ runPut (putWord8 4) :: Reliability
*** Exception: Data.Binary.Get.runGet at position 1: Reliability.get: invalid

If you are talking about this issue generic instance have exactly the same properties in this sense, this is also partial and can't decode values that are not represented by Reliability universe:

*Network.Transport Data.Binary.Put Data.Binary> decode $ runPut (putWord8 4) :: Reliability
*** Exception: Data.Binary.Get.runGet at position 1: Unknown encoding for constructor

Correct me if I'm wrong, but I thought that is some older versions of binary generic instance used Int (size 4) in order to represent such types, but I've checked that and see that it has (size 1).

Summary: I'm not opposed to this PR (because resulting serialization is the same, but we keep lesser amount of code), but I'm not sure I totally understand an impact.

@izgzhen
Copy link
Contributor Author

izgzhen commented May 28, 2015

@qnikst sorry for poor information I provided about my intention, the "completeness" I mentioned is that, if we'd like to serialize Event, it might be required to be the instance of Binary, so this will depends on ErrorEvent, MulticastAddress etc. to be instance of Binary. Reliability certainly is also one of them, but for simplicity, I also changed the hand-written serialization of Binary into derived one.

So, the completeness I means is that, the Event can be serialized and is instance of Binary.

I am still a Haskell learner, so if there is any mistake or overlooking please just point it out :)

@qnikst
Copy link
Contributor

qnikst commented May 31, 2015

Ah.. great, sorry for misunderstanding.

Could you, please, split PR's into 2 commits (or better PR), one with Binary instances for all types that you are missing, and another with using generic deriving for Reliabitity. Latter could be build on the top of the former one. I definitely opened to adding missing instances. And I'm open to move to generic if project built with all supported ghc's use Word8 in order to tag type constructor in instances, so effectiveness (and even interface) will be preserved (if you don't have a time I could check that on my own).

@izgzhen
Copy link
Contributor Author

izgzhen commented May 31, 2015

OK, I will check it now.

@izgzhen izgzhen closed this May 31, 2015
@qnikst
Copy link
Contributor

qnikst commented May 31, 2015

@izgzhen I have checked, generic Binary instances was always ok, so feel free to reopen.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants