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

Cleanup stream method guards #287

Merged
merged 7 commits into from
Apr 20, 2019
Merged

Cleanup stream method guards #287

merged 7 commits into from
Apr 20, 2019

Conversation

DanRStevens
Copy link
Member

@DanRStevens DanRStevens commented Mar 25, 2019

Updates for Write stream method guards.

This consolidates the std::vector and std::string variants of the write methods into unified container write methods. The differences were little more than variable names, and the overload type.

This relates to Issue #278. This simplifies template parameters, which may help PR #281.

Corresponding updates should be made to the Read stream method guards. (Added in PR #288).

The existing unit tests helped with the development of this. Particularly after I changed the meaning of a template T parameter, but forgot to update a corresponding sizeof(T). Nevertheless, I find myself thinking a few additional unit tests might be good, which runs through various categories of types, doing a quick round trip serialization on them.

Move the `std::vector` restriction into an `enable_if` clause. Use
member type definitions instead of template arguments.
Guard against the container itself being trivial to avoid ambiguous
calls. This excludes `std::array`, which is served by the other method
taking trivial types. A trivial container of trivial types is itself
trivial.

Note: Technically, this should guard for types with `.data()` and
`.size()` members. Such members imply a contiguous container.
This is now served by the more general non-trivial contiguous container
of trivial types case.
Move the `std::vector` restriction into an `enable_if` clause. Use
member type definitions instead of template arguments.
It doesn't really need to exist, and eliminating it makes the
implementation match the size prefixed string write method.
This can potentially handle the size prefix part of any container write,
including containers we don't yet support serialization for. Currently
we simply delegate for the details of writing the actual container
contents.
This is now served by the more general size prefixed container write
case.
Copy link
Member

@Brett208 Brett208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to compile and work fine. Tried with OP2Archive and it worked. I keep glazing over when trying to understand the significance.

@DanRStevens DanRStevens merged commit b7cac41 into master Apr 20, 2019
@DanRStevens DanRStevens deleted the cleanupStreamMethodGuards branch April 20, 2019 13:29
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 this pull request may close these issues.

2 participants