Add binary type support to all binary file formats, as well as an internally represented binary type

[OmnipotentEntity]

This pull request completely implements binary type serialization and deserialization for all formats that support it. And it adds an internally represented binary type for JSONs (non-standard and serializable by default only to binary formats). The internally represented binary type also carries along an optional binary subtype, which is used by certain binary formats (msgpack, bson)

UBJSON does not support binary types, and they recommend serialization to a list of numbers, which is what is implemented by this pull request. This has the side effect of not being roundtrippable. Similarly, cbor does not support binary subtypes, so the subtype field for cbor is also not roundtrippable. Otherwise, the implementation should be roundtrippable on all inputs.

This pull request addresses #1129.

(Code coverage 100% is checked, because I think I covered everything, I don't know if we're using coveralls anymore, and I don't know how to trigger a recheck. Please trigger a recheck or tell me how, and if my code coverage is lacking I'll add more tests.)

deserialization. Currently no tests and completely untested other than
for basic compilation

This is not a complete pull request as it stands, I have made no attempt to test or amalgamate the code yet. This is currently in the state it is in because I'd like feedback before going farther. This pull request is intended to address #1129 .

Code is thus far only tested as compiling using gcc 7.4.0 (however, because much of the code is templated, and I don't have any tests that actually calls the code, there are undoubtedly numerous compilation issues just waiting to say hi just as soon as I get around to it.)

Is this approach decent? Do you have any specific requests as far as implementation is concerned? Is there anything I'm doing that I ought not? Have I missed anything large?

---

Pull request checklist

Read the Contribution Guidelines for detailed information.

• Changes are described in the pull request, or an existing issue is referenced.
• The test suite compiles and runs without error.
• Code coverage is 100%. Test cases can be added by editing the test suite.
• The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

• The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
• Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
• Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
• Please do not open pull requests that address multiple issues.

[nlohmann]

Thank you very much! On first browsing, this looks all very good! Unfortunately, I won't be able to have a deeper look at this until next week.

[OmnipotentEntity]

That's totally understandable. Clearly I'm OK with things taking a long while. ;)

I'll patiently await your feedback before continuing.

[OmnipotentEntity]

If you don't have anything specific to critique about my general approach, then I'll keep at it for now. Let me know though.

[nlohmann]

Please go ahead! The approach appears sound and has great potential!

[OmnipotentEntity]

Does anyone have any good test cases for the various binary types? I'd rather not haaaaave to make them all by hand if I don't need to. ;)

[nlohmann]

Does anyone have any good test cases for the various binary types? I'd rather not haaaaave to make them all by hand if I don't need to. ;)

I can add examples.

Meanwhile, the build is red. It seems that you are using macros like JSON_UNLIKELY. This macro is now called JSON_HEDLEY_UNLIKELY as we now use Hedley under the hood. Can you please update?

[nlohmann]

Do you have an idea why the tests are failing?

[OmnipotentEntity]

Yeah, sorry I buried the reason in a commit message. When I make the json it calls the json array constructor instead of the json binary constructor, because the template match is better somehow. I was contemplating a solution, but I think the best course of action is to simply remove the unadorned binary ctor so you can't accidentally call it, and then make static functions that return a constructed json of a specific type. Something like json::make_binary(BinaryType foo), but I haven't gotten around to it yet, because my irl job has been busy.

[OmnipotentEntity]

I have a few cbor data tests for binary. But no messagepack, ubjson, or bson. Soliciting contributions to make sure my code doesn't suck. ;)

[coveralls]

Coverage decreased (-2.08%) to 97.922% when pulling 54d33b6 on OmnipotentEntity:develop into 973c52d on nlohmann:develop.

[nlohmann]

Thanks for the effort! I will have a look at this during the weekend!

[nlohmann]

Any idea how I can add tests to this PR? Should I make a copy of the branch and go ahead from there?

[OmnipotentEntity]

Typically the flow in Github is you make a fork of my branch of your fork, submit a pull request to me, I merge it there, and then you merge it here. But that's complicated and kind of dumb tbh.

In regular git, you'd just merge my diff into a different branch, change that branch, and then merge that branch into the standard branch all locally.

One thing that I did to fix the tests, which probably should be discussed a bit, is I added text serialization (but not deserialization) to binary values (as 'b[]'). This is because the testing harness really, really wants to print the value of json, and printing the value of the json is the same as serializing it. So it was throwing when it contained a binary value. I wanted to ask if I should add a deserialization or wrap the serialization code in a ifdef that only is compiled when running tests.

[OmnipotentEntity]

It looks like the failing travis test is a result of a failed apt-get command. Strange.

[nlohmann]

Some more comments.

[nlohmann]

It looks like the failing travis test is a result of a failed apt-get command. Strange.

I restarted the builds. Travis is flaky sometimes.

[nlohmann]

Typically the flow in Github is you make a fork of my branch of your fork, submit a pull request to me, I merge it there, and then you merge it here. But that's complicated and kind of dumb tbh.

In regular git, you'd just merge my diff into a different branch, change that branch, and then merge that branch into the standard branch all locally.

One thing that I did to fix the tests, which probably should be discussed a bit, is I added text serialization (but not deserialization) to binary values (as 'b[]'). This is because the testing harness really, really wants to print the value of json, and printing the value of the json is the same as serializing it. So it was throwing when it contained a binary value. I wanted to ask if I should add a deserialization or wrap the serialization code in a ifdef that only is compiled when running tests.

I'll try the fork of your branch tomorrow and open a PR then.

[OmnipotentEntity]

I'm intending on spending part of Sunday working on this. As far as I can tell all that is left is the test cases for msgpack, bson, and ubjson. Please let me know if this is correct. (And what you think of the serialization toggle.)

[nlohmann]

@OmnipotentEntity Sorry for not answering earlier. I will try to check this during the weekend.

[nlohmann]

I am really sorry. Something unexpected came up and I won't be able to work in this the next days. :-/

[OmnipotentEntity]

No worries, I understand life happens, and this is quite a large change that touches many, many different systems (so it's worth taking the time to make certain it's right). I'll try to get in some test cases for the other binary data types today, and then I'll wait for a review.

[OmnipotentEntity]

@nlohmann I think this pull request is finally ready.

[nlohmann]

@nlohmann I think this pull request is finally ready.

Cool! Thanks for keeping working on this! I will have a look soon!

[nlohmann]

Thanks a lot for your contribution! I add some very picky remarks for polishing the last details. The only API issue is the handling of subtypes - I think the current functions can all be const, but maybe I oversaw a use case for this.

As you see, I changed the @since version to 3.8.0 as I think this contribution will deserve it's own feature release. I am looking forward to this!

[OmnipotentEntity]

I guess Code Inspector is tired of this pull request lol

Anyway, I implemented your requested changes. Let me know if it looks good to you and I'll squash back to a single commit.

[nlohmann]

One minor change request. Thanks for the quick responses!

[OmnipotentEntity]

Good morning. I've squashed the commits.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks a lot! I am not waiting for AppVeyor and will merge this as soon as all CI is green.

[nlohmann]

THANKS!

[nlohmann]

---

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

• ✨ New Feature
• 🐛 Bug Fix
• ⚡️ Improvement
• 🔨 Further Change
• 🔥 Deprecated function

Description

• Added binary type. Add binary type support to all binary file formats, as well as an internally represented binary type #1662 CBOR byte string support #1129 Binary (0x05) data type for BSON to JSON conversion #1668 CBOR string #757 byte string support #483

---

[mdenna-nviso]

@OmnipotentEntity thanks a lot for this great contribution!