Update to recent RapidJSON and integrate streaming (was JSON-parser errors are very unhelpfull) #82
Comments
|
Example: Code: #include <fstream>
#include <iostream>
#include <string>
#include <vector>
#include <cereal/cereal.hpp>
#include <cereal/types/string.hpp>
#include <cereal/types/vector.hpp>
#include <cereal/archives/json.hpp>
int main(int argc, char** argv) try {
if(argc != 2) {
return 1;
}
std::vector<std::vector<std::string>> vec;
std::ifstream file{argv[1]};
cereal::JSONInputArchive archive{file};
archive(vec);
} catch(std::exception& e) {
std::cerr << e.what() << '\n';
return 2;
}JSON: {
"value0": [
[
["foo"]
],
[
"bar",
"baz"
]
]
}Error: |
|
It would be great if we could figure out how to add line numbers to errors as well. Ideally, the above error would read something like:
|
|
I think this would require modification of rapidjson as I can't find anything related to storing or retrieving that information in the source. Honestly moving to a different library wouldn't be such a bad idea in my opinion, rapidjson is nice but it does make some questionable decisions. For example using It might be worth the effort to extract the JSON reader/writer routines from chromium's source. It doesn't look like it'd be too hard as it's not really that tied down to the rest of chromium (you'd just need to remove google test and replace the type variant it uses with a standalone one). It already includes this information, is wicked fast and is battle tested. As a bonus the license is BSD-compatible which plays nice with cereal :). Facebook's folly library also has a JSON reader/writer which supports these features with an equally lovely license. You would need to extract the |
|
According to the discussion on reddit rapidjson is no longer maintained, so a change might be beneficial in more than one way. |
|
It's worth noting that folly is probably the better choice than chromium's as it's easier to make standalone and is more in line with cereal's design philosophy of using modern C++ and has an API similar to rapidjson for retrieving/casting values. Google also doesn't allow many C++11 features in it's code base, most notably move semantics and prefers functions like |
|
I like the idea of moving to one of these simpler looking parsers instead of using RapidJSON. |
|
I started toying around with extracting FB folly, the main problem is it uses That and I can't get gtest to compile under clang on OSX :(. It uses |
|
Another option: https://github.com/esnme/ujson4c/ |
|
Just as a side note, RapidJSON has never been maintained. The developer created it and that was it. I asked him once (per eMail) when a new version will come out and he asked what was missing.... so maybe you could contact him too and ask for specifics? |
|
Please note that RapidJSON has moved to GitHub and there are active efforts solving previous issues. Feel free to jot down issues or advices there. |
|
Hi @miloyip , great move and thanks for reviving the lib again. I really like it and want to see it evolve and support modern C++ standards and compilers. 👍 |
|
Considering the fact that RapidJSON now supports streaming, we'll likely stick with it and work on improving error messages and usability after we upgrade. |
|
By the way, we have improved the error reporting mechanism. Now parse error is retrieved as an http://miloyip.github.io/rapidjson/md_doc_dom.html#ParseError |
Jip, line numbers would be really good. Thanks. Side note: Regarding JSON and C++11: |
Updated and made changes necessary for the new version of rapidjson. Looks good on ubuntu under the compilers I can test with, needs MSVC testing. We had some internal changes to rapidjson but these didn't seem necessary with the new version. Haven't done any performance testing, initial estimates put it at nearly the same speed for json serialization. Could probably optimize things. relates #82, #121
|
We're now using RapidJSON 1.0.2, I haven't done anything other than make it work as is, so no streaming or anything like that yet. |
The error-messages in the exceptions thrown by the JSON-parser aren't very helpfull:
It would be great if these could become more descriptive, maybe something like this:
The text was updated successfully, but these errors were encountered: