-
Notifications
You must be signed in to change notification settings - Fork 46
Fix build with Boost 1.86.0. #1247
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
base: master
Are you sure you want to change the base?
Conversation
I've asked our Legal folks for permission to sign the CLA. |
Legal gave me permission, CLA now signed :) |
2197375
to
f27f5c4
Compare
PR closed by Hazelcast automation as no activity (>3 months). Please reopen with comments, if necessary. Thank you for using Hazelcast and your valuable contributions |
@JackPGreen Could you reconsider looking at this? If hazelcast can't be built with current boost we are going to be forced to remove it from our curated registry. |
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.
Thanks for the PR. It is great findings. I have a single comment.
@@ -594,7 +599,7 @@ class HAZELCAST_API ClientMessage | |||
std::is_same<std::pair<typename T::value_type::first_type, | |||
typename T::value_type::second_type>, | |||
typename T::value_type>::value && | |||
std::is_trivial<typename T::value_type::first_type>::value && | |||
hazelcast::util::is_trivial_or_uuid<typename T::value_type::first_type>::value && | |||
std::is_trivial<typename T::value_type::second_type>::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.
std::is_trivial<typename T::value_type::second_type>::value, | |
hazelcast::util::is_trivial_or_uuid<typename T::value_type::second_type>::value, |
there are 6 places in ClientMessage.h which uses std::is_trivial
and all needs to change right?
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.
I have to admit I don't really know what this code does; I just checked where they presumably were supposed to go before.
A use of is_trivial
only needs to change if you expect that thing to accept boost::uuid
in that place.
I think I got all the get
overloads, or did I miss one?
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.
@BillyONeal Just sent an update commit for the missing parts. Can you review them if you see any problems. And then, we can merge if the PR is green. It would still be good to see if this code would work in latest boost versions.
@@ -373,7 +373,7 @@ class HAZELCAST_API SerializingProxy | |||
} | |||
|
|||
template<typename T> | |||
typename std::enable_if<std::is_trivial<T>::value, boost::future<T>>:: |
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.
I don't think the changes in this file have any effect because nobody tried to use this decode
function with a boost::uuid. No objection to the change though.
@@ -600,7 +600,7 @@ class HAZELCAST_API ClientMessage | |||
typename T::value_type::second_type>, | |||
typename T::value_type>::value && | |||
hazelcast::util::is_trivial_or_uuid<typename T::value_type::first_type>::value && | |||
std::is_trivial<typename T::value_type::second_type>::value, | |||
hazelcast::util::is_trivial_or_uuid<typename T::value_type::second_type>::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.
I didn't touch this one or the one after because as far as I could see nobody formed vector<pair<trivial, uuid>>
but no objection to the change.
Unfortunately, it looks like 1.87 broke even more stuff :( |
I'm not sure if you've spotted something locally, or if it's the failing CI builds - if the latter, this PR has flagged some separate issues that we're addressing in parallel :) Thanks for the contribution! |
I mean microsoft/vcpkg#42678 which tries to update vcpkg to 1.87. We are likely to remove hazelcast-cpp-client from our registry in order to take the boost update (and will of course add it back as soon as we have a hazelcast-cpp-client that successfully builds) |
It is this reported issue. We have to look for upgrading boost to a more recent version as minimum required version and fixing the related problems. |
Looks like this landed, which deindexed |
Fixes #1245
boost::uuid::uuid
is no longer trivial, anddata
is no longer an array, but a type providing overloaded operators that attempt to "look like" an array.These changes should be safe on previous boost versions.