Some tweaks for reading/writing#1609
Some tweaks for reading/writing#1609helly25 wants to merge 2 commits intoopen-source-parsers:masterfrom
Conversation
* Make `json.h` an IWYU import header. * Change `Reader::parse` to take its `document` parameter as `std::string_view`. * Add `static void StreamWriterBuilder::updateDefaults(const Json::Value& settings);` * Allows to set the global configuration.
|
@baylesj could you review this? |
Thanks for this patch. I have (obviously) not been super active in this repository as of late but am intending to provide a little bit more active stewardship. I am happy to review. |
| * error occurred. | ||
| */ | ||
| bool parse(const std::string& document, Value& root, | ||
| bool parse(std::string_view document, Value& root, |
There was a problem hiding this comment.
This causes a compile time error in the build since we don't support string_view currently. I believe JsonCpp is on C++11? And std::string_view was added in C++17.
We have been talking about a major version change so it's quite possible bumping the standard could be part of that version change. But right now I don't think we can land this specific part of the change unfortunately.
| char const* end = begin + doc.size(); | ||
| // Note that we do not actually need a null-terminator. | ||
| CharReaderPtr const reader(fact.newCharReader()); | ||
| return reader->parse(begin, end, root, errs); |
There was a problem hiding this comment.
Nit: I think it's fine to have begin and end inline here:
return reader->parse(doc.data(), doc.data() + doc.size(), root, errs);
Seems clear enough to me.
| // static | ||
| void StreamWriterBuilder::setDefaults(Json::Value* settings) { | ||
|
|
||
| static Json::Value& global_settings_ = *new Json::Value([] { |
There was a problem hiding this comment.
This is declaring a new global static, which may have implications for some libraries around storage and memory. For example, in Chrome, global statics like this aren't usually allowed.
Is there a performance or other benefit causing this change? I'm not aware of any.
2 participants
Some tweaks:
json.han IWYU import header.Reader::parseto take itsdocumentparameter asstd::string_view.static void StreamWriterBuilder::updateDefaults(const Json::Value& settings);