-
Notifications
You must be signed in to change notification settings - Fork 4
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
344 implement user traits #345
base: develop
Are you sure you want to change the base?
Conversation
45d2aca
to
28e3ef9
Compare
014e8dd
to
02b04a2
Compare
@Matthew-Whitlock will this require some changes to VT? |
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.
@Matthew-Whitlock PR tests with vt
build is currently failing in the configuration stage and reports success incorrectly. #352 fixes both these issue, please rebase once it is merged.
02b04a2
to
274138b
Compare
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.
About half reviewed
cf77dcb
to
772f17c
Compare
772f17c
to
5eecca8
Compare
5eecca8
to
783da5f
Compare
783da5f
to
f7ff36d
Compare
@Matthew-Whitlock Can you rebase and mark as ready for review? |
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.
Whew, sorry it took so long I added a bunch comments
src/checkpoint/traits/user_traits.h
Outdated
struct NoTrait; | ||
|
||
template<typename Traits, typename T, typename... U> | ||
struct without_helper { |
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.
Here and elsewhere in the PR make sure the casing matches the rest of magistrate, i.e. CamelCase
for struct names
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.
Happy to make this change, but I think underscore casing makes sense for type manipulation and template helpers. The rest of the code is a bit inconsistent, though.
serializable_traits.h has:
template <typename U>
using specialized_reconstruct_t = decltype(
checkpoint::CheckpointReconstructor<U>::reconstruct(
std::declval<U*&>(),
std::declval<void*>()
)
);
using has_specialized_reconstruct =
detection::is_detected<specialized_reconstruct_t, T>;
//But also this style
template <typename U, typename V>
using serializeThis_t =
decltype(std::declval<U>().serializeThis(std::declval<V&>()));
using has_serializeThis = detection::is_detected<serializeThis_t, T, S>;
checkpoint_traits.h:
template <typename U>
using contiguousBytes_t = decltype(std::declval<U>().contiguousBytes(
std::declval<void*>(), std::declval<SizeType>(), std::declval<SizeType>()
));
using has_contiguousBytes = detection::is_detected<contiguousBytes_t, T>;
reconstructor_traits.h:
template <typename T>
using isDefaultConsType =
std::enable_if_t<std::is_default_constructible<T>::value>;
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.
Hah, I'll defer to @lifflander then for this one :P
} | ||
|
||
struct ShallowTrait {}; | ||
struct TraitPairA {}; |
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.
TraitPairA
is a little confusing as they don't seem the be pairs?
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.
The naming is based on the check in UserObjectA that expects to have either both or neither of TraitPairA and TraitPairB
auto serB = checkpoint::serialize<UserObjectB, ShallowTrait>(objB); | ||
objB.val_b = new_b_val; | ||
objB.val_a = new_a_val; | ||
checkpoint::deserializeInPlace<UserObjectB, ShallowTrait>(serB->getBuffer(), &objB); |
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.
should this test also check name to ensure that the UserObjectA was called correctly
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.
We can, but I don't think that's checking for anything that wouldn't already be caught by test_serialize_extra so I'd rather keep the checks within the test's 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.
With the extra check for correct trait detection, we should be fine leaving the name checking to the test_serialize_extra test
auto serB = checkpoint::serialize<UserObjectB, TraitPairA>(objB); | ||
objB.val_b = new_b_val; | ||
objB.val_a = new_a_val; | ||
checkpoint::deserializeInPlace<UserObjectB, TraitPairA>(serB->getBuffer(), &objB); |
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
e721da2
to
a8bd64c
Compare
a8bd64c
to
3c73754
Compare
Closes #344