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.

Sign up for GitHub

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

Jump to bottom

initial import of valarray serializer #2714

Merged
merged 11 commits into from Jun 29, 2017
Merged

initial import of valarray serializer #2714

merged 11 commits into from Jun 29, 2017

Conversation

ct-clmsn
Copy link
Contributor

serializer for std::valarray

@hkaiser hkaiser added this to the 1.1.0 milestone Jun 25, 2017
@hkaiser
Copy link
Member

hkaiser commented Jun 25, 2017 •
edited

Chris: thanks for submitting this!

@ct-clmsn
Copy link
Contributor Author

Anytime! Hoping this is the beginning of future contributions with a fair bit more complexity. Will begin making the requested modifications, thank you for the opportunity to contribute!

@ct-clmsn
Copy link
Contributor Author

The unit test is a copy+paste+modification of the std::array unit test.

@hkaiser
Copy link
Member

hkaiser commented Jun 25, 2017 •
edited

@ct-clmsn: The test will run only if it was added to the CMakeLists.txt file here: https://github.com/STEllAR-GROUP/hpx/blob/master/tests/unit/serialization/CMakeLists.txt#L6-L22

@ct-clmsn
Copy link
Contributor Author

@hkaiser thank you for the reminder!

hkaiser
hkaiser reviewed Jun 28, 2017
View reviewed changes
hpx/runtime/serialization/valarray.hpp Outdated

template<typename T>
void serialize(hpx::serialization::output_archive &ar,
const std::valarray<T> arr, int /* version */)
Copy link
Member

Choose a reason for hiding this comment

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

Chris, sorry for still having something to complain about... Here, arr should be passed by const& to avoid unnecessary copying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a problem, got a bit caught up sorting out text formatting issues. Thanks!

hkaiser
hkaiser approved these changes Jun 28, 2017
View reviewed changes
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks a lot!

@hkaiser hkaiser merged commit 383eab9 into STEllAR-GROUP:master Jun 29, 2017
@hkaiser
Copy link
Member

hkaiser commented Jun 29, 2017

Chris: congrats to your first merged PR! This qualifies you for a STE||AR-Group T-shirt ;)

@ct-clmsn
Copy link
Contributor Author

ct-clmsn commented Jun 29, 2017 •
edited

@hkaiser you are too kind! thank you for the support and the opportunity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkaiser hkaiser hkaiser approved these changes

Assignees
No one assigned
Labels
category: serialization type: enhancement
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ct-clmsn @hkaiser