Seamlessly support non-default-constructable custom class deserialization#1087
Seamlessly support non-default-constructable custom class deserialization#1087mzoll wants to merge 17 commits into
Conversation
…imple return type instead
…able custom types, while seamlessly supporting the old API; see Tutorial.mp for details
…able custom types, while seamlessly supporting the old API; see Tutorial.mp for details
… of clearer code structure
SGSSGene
left a comment
SGSSGene
left a comment
There was a problem hiding this comment.
I see the need/desire for a converter that works for non defaultable types.
Changing the API is not an option. This would break to much and currently the goal is to maintain this library to keep it compatible and reduce number of bugs.
So it would need some mechanism that allows for old and new API to work together.
Additionally, I am not satisfied with the conversion from exception to return value paths:
try {
...
} catch(const conversion::DecodeException& e) {
return false; //not doing this breaks upstream functionality
} catch (...) {
throw;
}
This in my eyes should not be. In c++23 I would use std::expected and in c++17 std::optional not sure what to do in c++11. Maybe std::variant?
| Observe that in the above example the custom type is, decalred as | ||
| a struct, explicit default constructable and all its members are | ||
| exposed. For non default constructable types like |
There was a problem hiding this comment.
Minor typo:
| Observe that in the above example the custom type is, decalred as | |
| a struct, explicit default constructable and all its members are | |
| exposed. For non default constructable types like | |
| Observe that in the above example the custom type is, declared as | |
| a struct, explicit default constructable and all its members are | |
| exposed. For non default constructable types like |
| rhs.x = node[0].as<double>(); | ||
| rhs.y = node[1].as<double>(); | ||
| rhs.z = node[2].as<double>(); | ||
| return true; |
There was a problem hiding this comment.
Wrong return value. Should be rhs not true.
| return rhs; |
| NonDefCtorVec3(double x, double y, double z) | ||
| : Vec3() { this->x=x; this->y=y; this->z=z; | ||
| }; |
There was a problem hiding this comment.
Proper initialization of variables:
| NonDefCtorVec3(double x, double y, double z) | |
| : Vec3() { this->x=x; this->y=y; this->z=z; | |
| }; | |
| NonDefCtorVec3(double x, double y, double z) | |
| : Vec3{x, y, z} | |
| {} |
| return true; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
In general, maybe convert<Vec3> should keep the old API?
|
|
||
| // static Node decode(const Node& node) { | ||
| // throw std::runtime_error("this should not have been encountered"); | ||
| // Node rhs; | ||
| // rhs.reset(node); | ||
| // return rhs; | ||
| // } |
| class NonDefCtorVec3 : public Vec3 { | ||
| using Vec3::x; | ||
| using Vec3::y; | ||
| using Vec3::z; | ||
| public: | ||
| NonDefCtorVec3(double x, double y, double z) | ||
| : Vec3() { | ||
| this->x=x; | ||
| this->y=y; | ||
| this->z=z; | ||
| }; | ||
| bool operator==(const NonDefCtorVec3& rhs) const { | ||
| return x == rhs.x && y == rhs.y && z == rhs.z; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Needs same adjustments as in the Tutorial.md
| using Vec3::x; | ||
| using Vec3::y; | ||
| using Vec3::z; |
There was a problem hiding this comment.
Using statements are not required.
| using Vec3::x; | |
| using Vec3::y; | |
| using Vec3::z; |
2 participants
The current API of the
convert<T>::decodeconstruct is limited to default constructable structs with visible members only. This is limiting and does not follow RAII.The here patch allows to declare in the custom 'struct convert' for any type
Ta decode method by slightly different, but more instructive, signatureT convert<T>::decode(const Node& node). Within that methodTcan be constructed by the user from the data serialized fromnodeand the result returned without the need to manipulate the members of an uninitialized reference. Problems in the deserialization process are marked by throwing theDecodeException, effectively replacing the old convoluted bool return signaling deserialization success.While users are free to implement this new API the old API is still supported in static code-path switches based on the
decode-signature detected.The complete documentation is found at the bottom of
docs/Tutorial.mdtests have been added and pass.
This patch closes #993
This patch also shows an alternative solution to merge request #1010