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

string_type in binary_reader #941

Closed
kaidokert opened this issue Jan 28, 2018 · 3 comments
Closed

string_type in binary_reader #941

kaidokert opened this issue Jan 28, 2018 · 3 comments
Assignees
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@kaidokert
Copy link
Contributor

Feature Request

  • Describe the feature in as much detail as possible.
    Currently binary_reader requires using std::string, which is cumbersome when trying to work with custom non-standard string classes. In my specific usecase, embedded system and cbor parsing.

  • Include sample usage where appropriate.
    The rest of the header currently compiles with a custom string type stand-in with a very minimal standard interface part:

struct awfully_poor_str {
    static const size_t npos = static_cast<size_t>(-1);
    using value_type = char;
    void push_back(char);
    awfully_poor_str();
    awfully_poor_str(const char *);
};

Changing binary_reader to

using string_t = typename BasicJsonType::string_t;

makes it compile as well. Or is there a definite reason why it should use std::string there ?

BTW, on a cortex-m0 compile with custom vector and string types, exceptions/rtti off, and std::streams stubbed out i get about 3kB for the library code size right now. Intel's C tinycbor is about 2.5kB, and a smaller custom c++ cbor-only limited-features parser is <1kB

binary_stringtype.diff.txt

@nlohmann
Copy link
Owner

I think this is the first time I heard of a proper use of a string_t other than std::string. :-)

There is no specific reason for std::string there. The diff looks simple enough to be used. PRs welcome!

I can image that a CBOR-only parser can be smaller, and I would be happy to improve the library code in this regard. But since CBOR is only one aspect of the library, I'm not sure how much smaller it can get. Then again - any feedback is more than welcome!

@kaidokert
Copy link
Contributor Author

kaidokert commented Jan 28, 2018

There's room to get smaller with a custom std::map implementation .. which i didn't know where to borrow right off the bat. Most of the remaining code size beyond parsing itself is std::map's red-black tree implementation, plus some bits pulled in by shared_ptr, required by input_adapter_t and output_adapter_t that i think can probably be avoided too, with some thought.

I'll make a pull request with the diff. It would probably good to add some basic unit-tests compiling with minimal std::string and std::vector replacements, to ensure future changes don't break the interface contract for those

EDIT: i went looking and could borrrow a flat map from boost maybe as well, it'll probably be smaller and the speed doens't much matter in typical tiny payloads in my use cases.

kaidokert pushed a commit to kaidokert/json that referenced this issue Feb 1, 2018
nlohmann added a commit that referenced this issue Feb 1, 2018
Templatize std::string in binary_reader #941
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Feb 1, 2018
@nlohmann nlohmann added this to the Release 3.1.0 milestone Feb 1, 2018
@nlohmann
Copy link
Owner

nlohmann commented Feb 1, 2018

Fixed by merging #950.

@nlohmann nlohmann closed this as completed Feb 1, 2018
@nlohmann nlohmann self-assigned this Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants