[Core] Support serialization through (smart) pointers to abstract classes #13153
Conversation
When loading a serialized smart pointer (or an owning raw pointer), don't attempt to instantiate the corresponding data type when it is abstract. This is useful when passing a (smart) pointer to an interface class, which was not supported so far. Note that without the added check, a compile-time error would be raised when passing a (smart) pointer to an abstract class. Other changes include: - Added a member function to class `Serializer` to deregister data types for (de)serialization. - Added four unit tests that verify whether an instance can be saved and loaded through a (smart) pointer to an interface class.
rfaasse
left a comment
rfaasse
left a comment
There was a problem hiding this comment.
Nice addition to get the serialization of abstract classes to work! Apart from the mysterious build issues, I don't have any blocking comments or questions (since we also discussed it quite a bit already).
markelov208
left a comment
markelov208
left a comment
There was a problem hiding this comment.
Hi Anne, nice background to make everything serializable. I have no blocking comments. There are no any checks for the abstract classes, should they be added? Thank you.
matekelemen
left a comment
matekelemen
left a comment
There was a problem hiding this comment.
Nice, just a minor comment.
kratos/includes/serializer.h
Outdated
| pValue = Kratos::shared_ptr<TDataType>(new TDataType); | ||
| if constexpr (!std::is_abstract_v<TDataType>) { | ||
| if(!pValue) { | ||
| pValue = Kratos::shared_ptr<TDataType>(new TDataType); |
There was a problem hiding this comment.
Avoid double allocation if you know it's an std::shared_ptr.
| pValue = Kratos::shared_ptr<TDataType>(new TDataType); | |
| pValue = std::make_shared<TDataType>(); |
There was a problem hiding this comment.
Hi @matekelemen,
Thanks a lot for your review. I completely agree with your suggestion. In fact, I would like to get rid of a few more "naked" news, by using Kratos::make_intrusive and std::make_unique. However, when I tried your suggestion, compilation failed since a few classes don't have a publicly accessible default constructor (see classes NurbsCurveGeometry, NurbsSurfaceGeometry, and NurbsVolumeGeometry). As a result, std::make_shared cannot construct instances of these classes. I guess that the @KratosMultiphysics/technical-committee needs to decide about the desired behavior. If they would like to keep the constructors of the aforementioned classes private, using std::make_shared is not an option. If, however, they feel it is okay to make these constructors public, we can change the implementation as per your suggestion.
There was a problem hiding this comment.
Oh that's nasty. In that case, you can do smth like this in the meantime.
…ization-through-abstract-classes
Following the suggestion by Mate.
…ization-through-abstract-classes
|
@roigcarlo Just a friendly reminder that I'd like to have your opinion on this PR. Is it ready to be merged? Or would you like me to make additional changes? Thanks. |
roigcarlo
left a comment
roigcarlo
left a comment
There was a problem hiding this comment.
Hey, sorry for the late reply, I must have marked this as view by mistake. 👍 from my side
No worries. Thanks for the review. |
📝 Description
This PR adds a few compile-time checks that avoid attempting to instantiate abstract base classes while loading objects from a serializer. So far, it was not possible to use (smart) pointers to abstract base classes when serializing objects of derived classes. By adding a few compile-time checks, we now can supply (smart) pointers to abstract base classes to the existing (de)serialization code.
For each modified
loadfunction, a corresponding unit test has been created.To keep the scope of changes to the registry as local as possible during a test run, a test class is registered and deregistered as needed using the RAII idiom. To this end, class
Serializerhas been extended with a member function to deregister data types.