Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental support for binary type and CBOR (de)serialization #862

Closed
wants to merge 1 commit into from
Closed

Experimental support for binary type and CBOR (de)serialization #862

wants to merge 1 commit into from

Conversation

khabinov
Copy link

@khabinov khabinov commented Dec 8, 2017

First of all huge thanks for creating this library.

I saw a few issues #757, #601 and #483 where people discuss if this library can support CBOR's binary data type. Motivation for this pull request is to discuss how easy it would be to add support for such data type.

Binary data type is not natural for JSON, but many people have to deal with it, especially, when converting from CBOR to JSON.

In C++, though, we can represent contiguous binary arrays using std::string, std::vector<uint8_t>, std::list<uint8_t>, char* and whatnot. It could be up to the user how to represent and convert this type into JSON.

Right now the change is mostly about the API aspect (e.g. adding binary type), without actual implementation.

Comments are very welcome! Thanks!

@gregmarr
Copy link
Contributor

gregmarr commented Dec 8, 2017

I'm not sure about the rest of this, but std::list<> is a horribly wasteful container to use for storing bytes. The std::list<> container is a node-based container. It has at least two pointers per node, and with the padding, that's a distinct memory allocation of a minimum of 24 bytes for every byte of data on a 64 bit system.

Much better would be std::vector<uint8_t>, or maybe std::deque<uint8_t> if there's a lot of incremental additions at the end.

@nlohmann
Copy link
Owner

nlohmann commented Dec 8, 2017

I shall have a look at this after the 3.0.0 release next week.

@khabinov
Copy link
Author

khabinov commented Dec 8, 2017

@gregmarr: you're absolutely right. The point was to show how binary type can be added, so, if this idea gets approved, I can spend more time polishing the change. Thanks!

@khabinov
Copy link
Author

khabinov commented Dec 8, 2017

I removed my naive implementation in order to focus mostly on API side of the change.

@khabinov khabinov changed the title [Don't merge] [Comments requested] Experimental support for binary type and CBOR (de)serialization [Comments requested] Experimental support for binary type and CBOR (de)serialization Dec 8, 2017
@khabinov khabinov changed the title [Comments requested] Experimental support for binary type and CBOR (de)serialization Experimental support for binary type and CBOR (de)serialization Dec 12, 2017
@stale
Copy link

stale bot commented Jan 19, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 19, 2018
@stale stale bot closed this Jan 26, 2018
@nlohmann nlohmann reopened this Jan 26, 2018
@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 26, 2018
@nlohmann
Copy link
Owner

I think we should not extend the JSON value with a binary type, but we may think about a SAX or StAX like parsing Interface that has a callback for binary types.

@nlohmann
Copy link
Owner

Related: #971

@nlohmann
Copy link
Owner

This issue will be addressed with #971 with a callback for binary types. The JSON value itself will not be changed for this.

Thank you for the initial PR.

@nlohmann nlohmann closed this Feb 25, 2018
@NickeZ NickeZ mentioned this pull request Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants