Remove majority of JsonObject::get_raw() usage.
#51137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
actual-nh
added
[C++]
akrieger
force-pushed
the
less_raw
branch
4 times, most recently
from
54de899
to
a4d3c62
Compare
- Add offset to JsonObject::throw_error and JsonValue::throw_error - Add JsonValue::string_error - Make JsonArray::throw_error const like JsonObject - Add JsonIn::get_value and JsonArray::next_value - Add JsonValue::test_null
The original form required JsonIn, which forced many callsites to use get_raw to pass into it. Add overloads for JsonObject and JsonValue and refactor the common code into a helper function that uses an error callback lambda to perform the equivalent error messaging that the original JsonIn form did, regardless of the input. Then refactor get_raw callsites off get_raw.
This eliminates a big source of get_raw calls which ultimately aren't needed. The subclasses and class hierarchy all can just use JsonValue because they don't use the streaming functionality of JsonIn at all. The one exception was `text_style_check_reader::get_next`, but it was feasible to refactor still.
These all have to be refactored in one go, because the deserialize statics get put into one shared map. Fortunately all of the definitions and declarations are in the same few files, so the impact is fairly contained. This eliminates the last 'large' usage of get_raw. What remains can be dealt with one at a time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Infrastructure "Remove usage of get_raw() from read_from_json_string, generic_typed_reader, activity_actor"
Purpose of change
#50143 changes how json is loaded from a text based representation to a 'pre-parsed' binary based representation. A streaming interface made sense when the data could be physically streamed, but it is a foreign concept to the binary representation. Implementing a streaming interface on top of it proved challenging, and instead eliminating the need for a streaming API was the more successful path. To do that, we have to eliminate all usage of
JsonObject::get_rawin the codebase, which drops us out of the object oriented parsing model back into the streaming model.read_from_json_string,generic_typed_reader, andactivity_actorare all broad surfaces that either directly or indirectly depend onget_raw. All three required touching large chunks of code to atomically refactor them, but between them represent most usage ofget_rawin the code and the hardest ones to eliminate. Thus, they get a separate PR to deal with them.Since I have to extend the Json APIs to support this PR, I threw in a couple more minor changes to them that will be needed later.
Describe the solution
read_from_json_stringuses the raw JsonIn api just to throw an error at a particular offset. We extend the existing object oriented json APIs to accept an offset parameter to their throw_error api. Then we refactor the implementation to essentially type erase the input type so we can accept eitherJsonInorJsonValuefrom the callsite, as required. Then just remove all theget_rawcalls feeding into it. There are still places that directly pass in JsonIn without usingget_rawso we have to keep support for that.generic_typed_readersupports a reader class hierarchy with some standard overloaded functions, one of which accepts JsonIn. It was easy to refactor the hierarchy to support usingJsonValueinstead and avoid the rootget_rawcall.activity_actoruses a map of string to deserializer function, so all the functions had to have the same type. Fortunately all but one were trivial to convert to JsonValue, and the one oddball just needed to test a JsonValue's type before deserializing appropriately.Describe alternatives you've considered
I tried implementing JsonIn on top of a flexbuffer but after a long struggle it just wasn't working out. JsonIn is 'very stateless' whereas a flexbuffer iterator needs to keep just enough state around that transitioning to and from the object api to the streaming api was too difficult to get right. Plus philosophically it didn't make sense to me to keep JsonIn around.
Testing
Build and create/load games.
Additional context
Required for #50143.