Feature/replace jsoncons #535
Conversation
instead of fetching for jsoncons it fetches nlohmann json instead. because fabulist also relies on it, it has to be told not to fetch it.
|
The build system YML should not require changes but I'll leave that up to @capnkenny to confirm. The licence distributable file requires you to add the licence for the new dependency, and remove the jsoncons licence assuming I added it originally, and that's it. |
|
Just confirming - build system does not need to change and passes as-is |
Pheubel
requested review from
FiniteReality and
tannergooding
as code owners
| jsoncons::json j(jsoncons::json_object_arg); | ||
| std::vector<jsoncons::json> memberMetadataJson{}; | ||
| nlohmann::json j{}; | ||
| nlohmann::json memberMetadataJson{}; |
There was a problem hiding this comment.
Is there a reason this isn't a std::vector of the json object anymore?
There was a problem hiding this comment.
The json object behaves like std::vector, i feel like having it be a json object instead would make more sense in showing what it actually is.
There was a problem hiding this comment.
To confirm, do we have a test covering this code path in the case the vector would have multiple entries?
|
|
||
| newMemberJson["name"] = member.name; | ||
| newMemberJson["type"] = static_cast<uint32_t>(member.type); | ||
| newMemberJson["type"] = member.type; |
There was a problem hiding this comment.
Is member.type already a uint32_t for nlohmann::json?
There was a problem hiding this comment.
peeking at the json header file, it does seem to already look at the underlying value in the enum, but i could be wrong. it is not the easiest library to look through
There was a problem hiding this comment.
It's converted to an int by default, but it can be customised: https://json.nlohmann.me/features/enum_conversion/
I would suggest doing so, as this avoids potential reordering errors.
| obj["location"].as<size_t>(), obj["sizeOfTypeInBytes"].as<size_t>(), | ||
| obj["length"].as<size_t>(), obj["sizeOfSerialisedDataInBytes"].as<size_t>()}; | ||
| BinaryMemberMetadata newMemberMetadata{obj.value<std::string>("name", std::string()), | ||
| obj.value<BinaryDataType>("type", BinaryDataType::NullOrUnknown), |
There was a problem hiding this comment.
What are the possibly BinaryDataTypes here?
There was a problem hiding this comment.
CC @RubyNova - should we be typing as UInt32 here? I see the static cast has been dropped, but I'm not sure if its necessary anymore (CI appears to pass but idk if we specifically test this)
| target_compile_options(nlohmann_json::nlohmann_json | ||
| INTERFACE | ||
| $<BUILD_INTERFACE:$<$<CXX_COMPILER_ID:MSVC>:/external:I${json_SOURCE_DIR}/include>> | ||
| $<$<CXX_COMPILER_ID:MSVC>:/external:W0> |
There was a problem hiding this comment.
Was this taken from Fabulist's version of the nlohmann::json dependency CMakeLists?
There was a problem hiding this comment.
yeah it was, i did bump the version, as fabulist is a little behind.
There was a problem hiding this comment.
Not a problem - my only ask is that we double check how this aligns with the Engine's compiler flags? I believe there's a difference between the two and I'm not sure which we prefer...
Cc @RubyNova for compiler flag input
There was a problem hiding this comment.
Since it is an external dependency I am unsure if it matters what flags this dependency requires.
You're best off asking @FiniteReality .
There was a problem hiding this comment.
Ideally we'd be using this format going forward, if I understand correctly. The /external:W0 here is just used to turn off warnings for external code.
|
Merging on @RubyNova's instructions. Fingers crossed this doesn't break everything 😜 |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Replaces jsoncons with nlohmann json as json provider for NovelRT
Is there an open issue that this resolves? If so, please link it here.
This PR partially resolves #500, more specifically; jsoncons was causing issues when compiling for C++ 20.
What is the current behavior? (You can also link to an open issue here)
The
DesktopResourceLoaderuses jsoncons for bson deserialization/serializationWhat is the new behavior (if this is a feature change)?
The
DesktopResourceLoaderuses nlohmann json for bson deserialization/serializationDoes this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
If users relied on jsoncons as json provider, they will have to either set up the inclusion themself, or convert their json code to work with nlohmann json.
If users relied on nlohmann json as json provider (indirectly provided through Fabulist) then they should not have to change their code.
Other information:
So far I only touched the CMake and the C++ code. I do not really know what exact alterations I would have to make to the LICENCE-DIST.md and the build-system.yml.