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
Serializing collections with non-default constructible data #2805
Conversation
value_type& ref = reinterpret_cast<value_type&>(storage); | ||
load_construct_data(ar, &ref, 0); | ||
ar >> ref; | ||
collection.push_back(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
collection.push_back(std::move(ref));
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks!
sizeof(value_type), alignof(value_type)>; | ||
|
||
collection.clear(); | ||
while (size-- > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to call collection.reserve(size);
here (at least for vector
and deque
etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note, thanks!
template <class Archive, class T> | ||
void save_construct_data(Archive&, T*, unsigned) | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a HPX_ASSERT(false);
if this function is supposed to never be called.
template <class Archive, class T> | ||
void load_construct_data(Archive&, T* t, unsigned) | ||
{ | ||
::new (t) T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, please add HPX_ASSERT(false);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks1
@krivenko could you please verify whether this is solving your issue? |
@AntonBikineev will you have time to look into @krivenko's problems or do you want me to do that? |
@hkaiser, yeah, I’m looking into this…
|
2e90fd5
to
bd92900
Compare
@krivenko please let us know if you're ok with these changes. |
@hkaiser I don't have permissions to merge this PR, could you do this please (if you feel it's ready)? |
@AntonBikineev I will do that. Let's get some of the other, older PRs in first. |
@AntonBikineev Anton, sorry for getting back to this, but why did you use a different mechanism to construct an instance of non-default-constructable types here than we've been using for serializing single instances of such types? |
@hkaiser, hm..., it seems like I just looked at what Boost.Serializaiton does. Having the same mechanism for deserializing collections and single instances would be definitely better.
… On Aug 18, 2017, at 5:04 PM, Hartmut Kaiser ***@***.***> wrote:
@AntonBikineev <https://github.com/antonbikineev> Anton, sorry for getting back to this, but why did you use a different mechanism to construct an instance of non-default-constructable types here than we've been using for serializing single instances of such types?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#2805 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFrQvm9w0utys7rM9yziIs2n_nC0PqQiks5sZZn3gaJpZM4OwCG1>.
|
@AntonBikineev see #2849 for how I propose to unify things. Please have a look. |
This fixes #2795