Review: std.msgpack

[Robert Jacques]

On Mon, 21 Jun 2010 01:22:40 -0400, Masahiro Nakagawa  
<repeatedly at gmail.com> wrote:
> On Sun, 20 Jun 2010 02:08:07 +0900, Robert Jacques <sandford at jhu.edu>  
> wrote:
>
>> On Sat, 19 Jun 2010 00:02:28 -0400, Masahiro Nakagawa  
>> <repeatedly at gmail.com> wrote:
>>
>>> mp_Object is based on std.json.JSONValue.
>>> Variant can't have mp_pack method.
>>> Hmm..., I check Variant again.
>>
>> Well, std.json is an incomplete, alpha library that's buggy and  
>> unreviewed. For example, the design is currently very C-ish with  
>> structs, enums, functions and exceptions all being independent top  
>> level constructs.
>
> I checked Variant, but Variant[Variant] doesn't work... :(

I know. But then again, how would you define a total ordering for Variant?  
The sadder aspect is that Variant[string] doesn't work either. Anyways, I  
primarily trying to recommend the API, although fixing Variant is the  
better long term solution.

For what it's worth, this works:
   Variant[Variant] bar = [ Variant("foo"): Variant(5),  
Variant(5.5):Variant(6) ];

But this doesn't
   Variant a = "bar";
   Variant b = 5;
   bar[a] = b;


>>> If Phobos has std.serializer like Orange(or Doost.Serialization?),
>>> I will implement MsgPackArchive.
>>> But general serializer usually is low-performance and lacks some  
>>> features.
>>> So when user wants performance, user will use Packer and Unpacker  
>>> directly.
>>
>> I'd disagree, with regard to performance: there's no reason why a  
>> general (cycle free) serializer would be slower than your direct  
>> routines. A serializer's only job is to pack/unpack complex objects  
>> correctly: the key advantage is that polymorphic objects/struct only  
>> have to support one custom serialization routine/mixin. Also,  
>> separating physical-encoding  from logical-encoding allows for some  
>> very powerful idioms. You could, for example, add cycle  
>> detection/support to msgpack by having the serializer write in a  
>> micro-format on-top of msgpack. Another, more important example is  
>> letting the user decide between an array and a map representation for  
>> structs/objects. Using arrays for objects is extremely brittle. If say,  
>> a new hire refactors a class from A{ int x; int y;} to A{ int y; int x;  
>> } you now have an extremely hard to find and track down logic bug in  
>> your program. To say nothing about trying to upgrade an implementation.  
>> On the other hand, using maps for objects adds a decent amount of  
>> overhead to the total serialized length.
>
> Yeah, It's a implementation-dependent problem. So I wrote "usually".
> If Phobos supports good serializer, I will remove Packer and  
> direct-conversion Unpacker.
>
>>>> Other little things,
>>>> -Why isn't nil mapped to null?
>>>
>>> Where?
>>
>> MsgPack has a nil type which I assume is equivalent to null. So why  
>> isn't it simply included in the object serializer (and arguably also in  
>> the array and map serializer as well)?
>
> Sorry, I can't understand this statement. Nullable types(class, array  
> and associative-array)
> is serialized to nil if null.

Sorry. The comment was about redundancy. i.e. packNil <=> pack(null).

>> For that matter, packNil, packTrue, packFalse, etc are all redundant  
>> with pack(T).
>
> I removed packTrue, packFalse.
>
>> Furthermore, stream, packArray, packMap and packRaw all should be  
>> internal routines. (Though, a rawPack(T)(T value) routine might not go  
>> amiss)
>
> I moved packRaw to internal method, but packArray and packMap should be  
> public.
> MessagePack's array and map can contain any type.

I can see the usefulness of the functionality, but for an end-user API, it  
is brittle and error-prone. For example:  
p.packArray(3).pack(5).pack("six"); produces an incorrect msgPack stream.  
A better syntax would be: p.pack(5,"six"); or p.packArray(5,"six"); I know  
you're using pack(Types...) to mean pack(Types[0]).pack(Types[1])... but  
using it for variable type arrays is worth considering. For packMap, pack(  
["key": "value"], [5:"six"] ) could work, but packMap( ["key": "value"],  
[5:"six"] ) or packMap("key","value",5,"six") would avoid confusion with  
pack(Types...).

>> (Returns: this to method chain. => Returns: self. i.e. for method  
>> chaining )
>
> Changed.
>
>> Oh, as for packing real you'll need to use CustomFloat!(80) for cross  
>> platform inter-op. Also, it shouldn't be @system: there's nothing  
>> memory-unsafe about writing or reading them. Please don't abuse  
>> @system, etc for meanings other their intended meanings.
>
> I forgot this. CutstomFloat was already fixed by you and David in trunk.
> I added 80bit floating point support for non-x86 environment.

Don't forget that some x86 platform store 80-bit floats in more than  
80-bits. So you do need to check the size of real on x86 platforms as well.

>
> Masahiro

P.S. Don't forget to apply changes to packer's API to unpacker.