-
-
Notifications
You must be signed in to change notification settings - Fork 427
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
Adding util::checkpoint #2917
Adding util::checkpoint #2917
Conversation
- Adding an operator== overload to checkpoint - suppressing an unused error warning
- Changing checkpoint test to support docs
- Update example to use std::copy and iterators to write to a file
- Adding operator!= - Make data a private member - Work to remove .load() - Replace with a contrutor that takes a char* - Change empty constructors to = default - Adding friend classes to handle data's new private status
- Cleaning up code - Fixing test/unit/util/checkpoint.cpp - Adding documentation for stream operators and access iterators
Updating add_checkpoint branch
hpx/util/checkpoint.hpp
Outdated
int const sequencer[] = {//Trick to expand the variable pack | ||
(ar << ts, | ||
0)...}; //Takes advantage of the comma operator | ||
(void) sequencer; // Suppress unused param. warnings |
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.
For MSVC you'll have to write this as:
int const sequencer[] = {
0, (ar << ts, 0)...
};
this avoids generating a possibly zero-sized array this compiler is complaining about.
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.
Will fix!
hpx/util/checkpoint.hpp
Outdated
checkpoint>::value>::type> | ||
hpx::future<checkpoint> save_checkpoint(T&& t, Ts&&... ts) | ||
{ | ||
{ |
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.
What's the rationale for introducing the extra scope?
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.
Leftover code. Should I remove all of these extra scopes?
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.
Removed per discussion
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.
Generally, LGTM, thanks! Please address the minor comments before merging.
hpx/util/checkpoint.hpp
Outdated
{ | ||
data.push_back(*stream); | ||
stream++; | ||
} |
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.
You should resize the vector and then memcpy the data into it here
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.
Yes, I agree - the signatures of these should be:
checkpoint(std::vector<char> const& stream);
checkpoint(std::vector<char> && stream);
or if you really must,
template <typename InIter>
checkpoint(InIter begin, InIter end);
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.
@hkaiser and I are discussing options to remove/replace this construction entirely as exposing pointers in the interface is a weak design.
hpx/util/checkpoint.hpp
Outdated
hpx::future<checkpoint> save_checkpoint(T&& t, Ts&&... ts) | ||
{ | ||
{ | ||
return hpx::dataflow(detail::save_funct_obj(), |
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 we use the io service pool here?
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 we use the io service pool here?
@sithhell what for?
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.
?
There are quite a lot of commits that only fix inspect errors / formatting / whitespace etc. |
- Fix for MSVC "zero-sized array" issue - Removing unnecessary scopes - Removing constructor with char* and replacing it with a constructor which takes a std::vector<char>
This pull request creates a new utility "checkpoint" which simplifies application checkpointing.