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

Add serialization for std::set (as there is for std::vector and std::map) #1658

Merged
merged 8 commits into from Jul 16, 2015
Merged

Conversation

gentryx
Copy link
Member

@gentryx gentryx commented Jul 15, 2015

No description provided.

@hkaiser
Copy link
Member

hkaiser commented Jul 15, 2015

@gentryx
Copy link
Member Author

gentryx commented Jul 15, 2015

Oops. My bad, my name. Changed to "ae"

for (std::size_t i = 0; i < size; ++i) {
T t;
ar >> t;
set.insert(set.end(), t);
Copy link
Member

Choose a reason for hiding this comment

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

You should make use of move semantics here.

@gentryx
Copy link
Member Author

gentryx commented Jul 15, 2015

All comments should be addressed by now. Once this is merged I'll follow up with a separate PR to update map.hpp and vector.hpp.

@AntonBikineev
Copy link
Contributor

@sithhell using size_t shouldn't be a problem and user doesn't need to be aware of it, because it's explicitly casted to int64_t/uint64_t in archives' code.

boost::uint64_t size = set.size();
ar << size;
if(set.empty()) return;
for (typename std::set<T, Allocator>::iterator i = set.begin(); i != set.end(); ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not look right, Allocator is the last template argument. Why not write this as

for (T const& elem: set) ar << elem;

to avoid problems?

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@hkaiser
Copy link
Member

hkaiser commented Jul 15, 2015

@AntonBikineev I hope it does not get casted to a 32bit but a 64bit value inside the archive..

@AntonBikineev
Copy link
Contributor

@hkaiser any integer always gets casted to 64 bit value. BTW, that behaviour may lead to narrowing when serializing something like boost::uint128_type.

@gentryx
Copy link
Member Author

gentryx commented Jul 15, 2015

@AntonBikineev So if I have 1000 int they'll take up 8k, not 4k? Is that what we want?

@sithhell
Copy link
Member

Am 15.07.2015 8:05 nachm. schrieb "Anton Bikineev" <notifications@github.com

@sithhell using size_t shouldn't be a problem and user doesn't need to be aware of it, because it's explicitly casted to int32_t/uint32_t in archives' code.

That sounds awfully wrong, I hope that's not the case. We've been bitten by
using size_t inside of serialization before. There shouldn't be any code
that's used for serialization that relies on std::size_t.

@AntonBikineev
Copy link
Contributor

@Sithell my understanding is following. when you use

size_t size;
ar << size

it calls

template <class T>
typename boost::enable_if<boost::is_integral<T>::value || 
    boost::is_unsigned<T>::value>::type save(T t) // or so
{
    save(static_cast<boost::uint64_t>(t));
}

Sorry for the mess, here it is:
https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/runtime/serialization/output_archive.hpp#L144

@sithhell
Copy link
Member

On 07/15/2015 08:57 PM, Anton Bikineev wrote:

@Sithell my understanding is following. when you use

|size_t size; ar << size |

it calls

|template boost::enable_ifboost::is_integral<T::value ||
boost::is_unsigned::value> save(T t) // or so {
save(static_castboost::uint64_t(t)); } |

You are right ... my bad. Some issues we've had in the past with
serializing std::size_t was when values like std::size_t(-1) might
appear, which is dangerous when getting sent from a platform where
sizeof(std::size_t(-1)) is 4 bytes to a platform where it is 8 bytes.

@hkaiser
Copy link
Member

hkaiser commented Jul 15, 2015

BTW, that behaviour may lead to narrowing when serializing something like boost::uint128_type.

@AntonBikineev Could you verify what happens for uint128_t, please?

@hkaiser
Copy link
Member

hkaiser commented Jul 15, 2015

The PR LGTM now.

@AntonBikineev
Copy link
Contributor

@hkaiser yeah, just reproduced this with the help of debugger. A half of most significant bits are cut off. What do you think would be more appropriate here? Having overload for this or just leave it as is to not mess the code?

@hkaiser
Copy link
Member

hkaiser commented Jul 16, 2015

Could you please add an appropriate overload for platforms supporting that type?

hkaiser added a commit that referenced this pull request Jul 16, 2015
Add serialization for std::set (as there is for std::vector and std::map)
@hkaiser hkaiser merged commit 772dcf3 into STEllAR-GROUP:master Jul 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants