WhiZTiM / UbjsonCpp

A high performance C++14 library for effortlessly reading and writing UBJSON
24 stars 11 forks source link

This badly needs binary support for the streams #4

Open mlfarrell opened 8 years ago

mlfarrell commented 8 years ago

Binary read+write support. How many days is this out? If I don't have it soon I may have to write it myself

//else if(v.isBinary()) // k = append_binary(v); //currently not supported

WhiZTiM commented 8 years ago

Thanks for the issue.

Yes please, feel free to improve the library. It will be well appreciated.

Quite frankly, I had a bit of trouble understanding the binary specification when I started writing this library... And also noticed that the other implementations (the ones I tested) didn't even cater for binary.... Even the @Steve132's C implementation

Again, this statement on the site

The #1 goal of Universal Binary JSON is compatibility with JSON

in has a bit of contradiction with the binary spec...

If I have a binary value, I can convert it to a strongly typed container. Fine... The question here is, since UBJSON has no marker dedicated to binary types, when parsing, if I encounter a container on strongly typed int8s how will I know its binary? Because as far as the spec is concerned, the user can supply an array of int8s in his data...

Should I just assume every strongly typed container of 'int8` is binary? Or am I missing something here?

Thanks

WhiZTiM commented 8 years ago

Also, Out of the frustration I had with the logical structure of parsing UBJSON, I decided to write a variant spec of UBJSON in January, 2015 when I implemented this library... I didn't push the spec online because I wanted to see if UBJSON will address my frustrations, but I doubt it has. I will push the spec later this week... And keep you posted..

Before then, since I haven't touched these two in a long while, I will take out time this weekend to re-familiarize myself with UBJSON and this library again so that we can evolve this library/UBJSON further where possible

mlfarrell commented 8 years ago

I uncommented your code and ran a quick test and it seemed to run fine. Any thoughts on that?

mlfarrell commented 8 years ago

I think your marker of 'b' for binary is fine. That should help you distinguish it from an array of int8's. I'll let you know if I run into any trouble regarding the ambiguity with what I'm doing.

tnovotny commented 6 years ago

I am fairly new to the UBJSON stuff, so I may be off, but i think adding a binary marker is wrong. With the marker you are no longer UBJSON, you are UBJSON+. If I understand the spec correctly, then UBJSON has no direct support for binary data because it wants to remain JSON compatible. It merely has support for tunneling binary data as an array of int8, so the the client will always get it back as the array of int8 it was transported in.

Steve132 commented 6 years ago

UBJSON has a uint8 type that is distinct from int8, combined with typed and lengthed array, allows arbitary binary arrays to be embedded in the stream.

tnovotny commented 6 years ago

The main point I was trying to make is, when you add a nonstandard binary marker, you become incompatible because other UBJSON implementations won't be able to decode. Therefore the "UBJSON+".

This incompatibility should be off by default and instead a "typed and lengthed array" should be written.

enum class Marker : byte { ...
   Binary  = 'b',  //Extension
};

template<typename StreamType>
std::pair<size_t, bool> StreamWriter<StreamType>::append_binary(const Value::BinaryType& bin) {
   write(Marker::Binary);
    ...
}
tnovotny commented 6 years ago

Aparently you become Uncomplicated Binary Exchange Format (UBEXF) and not "UBJSON+"

WhiZTiM commented 6 years ago

Thanks for the feedback I will act on this,