Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle reconstruction of virtual serialized types sensibly #151

Closed
lifflander opened this issue Oct 27, 2020 · 8 comments · Fixed by #152
Closed

Handle reconstruction of virtual serialized types sensibly #151

lifflander opened this issue Oct 27, 2020 · 8 comments · Fixed by #152
Assignees

Comments

@lifflander
Copy link
Contributor

This will allow users to skip that if they allocate the object for themselves and don't need it.

@PhilMiller
Copy link
Member

I think I found a bug in allocateConstructForPointer in the non-virtual case, which is called by serialize(unique_ptr) - it reconstructs unconditionally, regardless of unpacking or not

@PhilMiller
Copy link
Member

There's a philosophical question this work raises - if we're unpacking into a pointer that has a pre-reconstructed object, of the right type, should we trust that its serializer will get it into the right state and thereby avoid reallocating, or should we always reallocate?

I'm inclined to say that a serialize method should be treated like the set of 'special' methods in C++ that have to maintain construction/lifetime invariants - i.e. copy constructors and assignment operators. Thus, it should be expected to always get the object into the right state from an unpacking serializer, regardless of how it may have been constructed or subsequently modified.

@PhilMiller
Copy link
Member

After thoroughly revising this in concert with the implementation of serialize(SerializerT s, unique_ptr p), I've found that there's no reason to actually expose the split. We can just choose not to allocate and overwrite in the apparent reconstruction case.

@PhilMiller
Copy link
Member

@bathmatt You may want to look at this also.

@lifflander
Copy link
Contributor Author

I think I found a bug in allocateConstructForPointer in the non-virtual case, which is called by serialize(unique_ptr) - it reconstructs unconditionally, regardless of unpacking or not

I believe the intention of allocateConstructForPointer is to only be called in the unpacking path by the user.

@lifflander
Copy link
Contributor Author

There's a philosophical question this work raises - if we're unpacking into a pointer that has a pre-reconstructed object, of the right type, should we trust that its serializer will get it into the right state and thereby avoid reallocating, or should we always reallocate?

I'm inclined to say that a serialize method should be treated like the set of 'special' methods in C++ that have to maintain construction/lifetime invariants - i.e. copy constructors and assignment operators. Thus, it should be expected to always get the object into the right state from an unpacking serializer, regardless of how it may have been constructed or subsequently modified.

I agree on this point. The serialize should guarantee that it gets the object back into the right state regardless of the starting point.

@PhilMiller
Copy link
Member

Discussion on EMPIRE call concluded that

  • we can keep the singular call, possibly with a rename
  • that call should internally handler isUnpacking and in-place deserialization
  • the call should be made unconditionally

PhilMiller added a commit that referenced this issue Nov 9, 2020
PhilMiller added a commit that referenced this issue Nov 9, 2020
#151: Split up virtual serialization logic and better support in-place deserialization
@PhilMiller PhilMiller changed the title Split allocateConstructForPointer into two function calls to separate type data serialization Handle reconstruction of virtual serialized types sensibly Nov 9, 2020
@PhilMiller
Copy link
Member

Addressed in #152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants