search

chadaustin / sajson

Lightweight, extremely high-performance JSON parser for C++11
MIT License
568 stars 43 forks source link

Removing std::string from errors #23

Closed iboB closed 7 years ago

iboB commented 7 years ago

I have a suggestion for a step towards removing the std::string dependency. Moving from error strings to error codes. You can see my implementation here: https://github.com/iboB/sajson/commit/5568a59286944dda72d8b554b20875ecf93b1654

If you like it, I'll add a pull request

chadaustin commented 7 years ago

Nice! I've been meaning to get around to eliminating that std::string dependency too - super glad you beat me to it.

I like the approach - the main things I'd change are the API:

The primary interface to the library shouldn't expose the details of the error code and argument mechanism IMO. I have mixed feelings about the error code - maybe there are users who would want programmatic access to the error, but the bulk by far will simply want to turn into a string. Since our errors are short, a format_error method on document that takes a char* and size_t would be great. (And for C++ folks, get_error_as_string that returns a std::string works too.)

Also, sajson.h should remain a single header file.

Thanks!

iboB commented 7 years ago

Ok. Here are the changes, based on the comments: https://github.com/iboB/sajson/commit/a3fd8f573d3e87bade48a2e231e1daba9c9727f1

This commit is based on the previous one

iboB commented 7 years ago

Accidentally sent the previous comment prematurely :)

... I have a use case where I need to check the errors in my code, and I imagine other people might too, therefore I kept the public (but this time _internal) getters of the error code.

chadaustin commented 7 years ago

Made some API tweaks and broke the sstream dependency but otherwise landed your changes -- you can see https://github.com/chadaustin/sajson/commit/6cd040c1f88a8c786ac06cdc45ec0bb830cab5b3

Thanks! Chad

iboB commented 7 years ago

Great! That's much better, indeed