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

Unpacking of containers default-constructs elements rather than using reconstruction logic #161

Closed
PhilMiller opened this issue Nov 20, 2020 · 4 comments · Fixed by #214
Closed
Assignees

Comments

@PhilMiller
Copy link
Member

For std::vector<T> v, the current code calls s | len; v.resize(len), which results in calling the default constructor T::T(). This may not be valid code at all, for some types. For other types, the default constructor could be needlessly expensive. This violates the usual stated contract serialization offers, that reconstruction should use a tagged constructor if one is defined.

@PhilMiller
Copy link
Member Author

PhilMiller commented Nov 20, 2020

I believe that the scrupulously correct strategy for types that should be treated with this care is to reserve sufficient capacity (see #162) , and then reconstruct elements one at a time, using calls to v.emplace_back(args). The ultimate implementation may act something like

for (size_t i = 0; i < size; ++i) {
  v.emplace_back(RECONSTRUCT_TAG);
  s | v.back();
}

However, that emplace_back call seems to be out of place. I think we actually want to centralize that logic in the reconstruction code, rather than duplicating/diffusing it to containers as well. Thus, I'll offer a straw-man proposal along the following lines (taking advantage of N3649, defined in C++14):

std::vector<SpecialType> v;
auto emplacer = [&v] (auto && ... args) { v.emplace_back(std::forward<decltype(args...)>(args...)); };
for (size_t i = 0; i < size; ++i) {
  dispatch::call_with_reconstruct_arguments_for<SpecialType>(emplacer);
  s | v.back();
}

Where that latter function is defined to come up with arguments of the same type as reconstruction does now, and pass them to the lambda it receives, rather than to an object constructor.

@lifflander
Copy link
Contributor

We already have classes (Reconstructor) that centralize this logic. We might need to extend it a little for this, but we do have an API that knows all the ways to reconstruct and tries them in a particular order.

@PhilMiller
Copy link
Member Author

My point was more that the container serializers uniformly don't call Reconstructor for the elements

@cz4rs
Copy link
Contributor

cz4rs commented Nov 24, 2020

suggested test case: use vector of a class with default constructor deleted

cz4rs added a commit that referenced this issue Dec 2, 2020
cz4rs added a commit that referenced this issue Dec 2, 2020
@lifflander lifflander assigned jstrzebonski and unassigned cz4rs Jun 7, 2021
jstrzebonski pushed a commit that referenced this issue Jun 18, 2021
jstrzebonski pushed a commit that referenced this issue Jun 23, 2021
jstrzebonski pushed a commit that referenced this issue Jun 24, 2021
jstrzebonski pushed a commit that referenced this issue Jul 16, 2021
jstrzebonski pushed a commit that referenced this issue Jul 16, 2021
jstrzebonski pushed a commit that referenced this issue Oct 8, 2021
jstrzebonski pushed a commit that referenced this issue Oct 8, 2021
jstrzebonski pushed a commit that referenced this issue Oct 8, 2021
jstrzebonski pushed a commit that referenced this issue Oct 8, 2021
jstrzebonski pushed a commit that referenced this issue Oct 13, 2021
jstrzebonski pushed a commit that referenced this issue Oct 15, 2021
…containers-elements

#161 use reconstruction logic for std::vector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants