search

point-platform / dasher

Fast, lightweight, cross platform serialisation tool
Apache License 2.0
7 stars 6 forks source link

TryReadBinary and Symbolic constants #8

Closed andysturrock closed 8 years ago

andysturrock commented 8 years ago

This is a bit of a hand merge. I slammed what you had done over the top of my changes and then reapplied some of the changes that I made that you hadn't. The intended result is:

  1. Implement TryReadBinary in the custom unpacker
  2. Change to using symbolic constants rather than magic hex values

On the second, I think it makes the code more readable. I only made the changes in the safe unpacker. Let me know if you think they improve the code and I'll do them throughout the rest. It's a bit of a fiddly search-replace.

drewnoakes commented 8 years ago

Hi Andy,

I've merged these ideas into master. This PR contains a load of redundant commits so I can't merge it directly. I've also done a few things slightly differently.

Firstly there's support for byte[] now. Previously this was supported via IReadOnlyList<byte> (which byte[] implements) however the indexer is a virtual method, so it's slower. byte[] is mutable, which is currently avoided in the lib, but sending binary data this way feels natural and useful enough to look the other way in this case. It also encodes more compactly (for bytes > 0x7f).

Constants seem like a good idea. I didn't piggy back off the public Format enum -- it means adding new members which don't actually represent formats, and makes for loads of casts. At first I couldn't see a 100% nice way of doing this, which is why I left the magic numbers everywhere :) Sometimes I wish there was an implicit conversion between the enum and its numeric type to avoid casts. An internal static class with consts seems nicer. There's also a small perf hit on comparisons vs bitwise masking, so I kept the previous semantics there but added constants to make it clearer to those who don't know how MsgPack encoding works.

andysturrock commented 8 years ago

Sorry about the merge - I knew it was a mess. I should have deleted my fork and started again I think. It would be useful if you could have multiple forks in github.

Yep agree on the enum - if you can explicitly state what the backing type is (in this case byte) there doesn't seem a reason not to do an implicit cast. Originally I used the MsgPackCode class from MsgPack-cli (which uses const ints) but I adapted it to use the enum you had added so we didn't end up with two definitions of the magic numbers.

OK cool on the byte[] thing.

drewnoakes commented 8 years ago

I think you can achieve what you're after using multiple branches in the same fork. A PR exists between two branches, so you just need to make sure the diff between those branches contains only the commits in question. That can be done through fetching, rebasing/cherry-picking and maybe force-pushing to your own fork. Also, you can amend the PR while it's open by updating the branch you want to merge from on your fork at any time before it's merged.

It seems every language handles enums differently. I'm really not sure what the best approach is.

On 24 December 2015 at 07:56, Andy Sturrock notifications@github.com wrote:

Sorry about the merge - I knew it was a mess. I should have deleted my fork and started again I think. It would be useful if you could have multiple forks in github.

Yep agree on the enum - if you can explicitly state what the backing type is (in this case byte) there doesn't seem a reason not to do an implicit cast. Originally I used the MsgPackCode class from MsgPack-cli (which uses const ints) but I adapted it to use the enum you had added so we didn't end up with two definitions of the magic numbers.

OK cool on the byte[] thing.

— Reply to this email directly or view it on GitHub https://github.com/drewnoakes/msgpack-strict/pull/8#issuecomment-166988640 .