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

fix behavior for deserializing sharedptr to const obj #418

Closed
wants to merge 2 commits into from

Conversation

pkrenz
Copy link
Contributor

@pkrenz pkrenz commented Jun 30, 2017

Fix for issue #417

@AzothAmmo
Copy link
Contributor

There are a few issues with your proposed changes, which look good overall.

  1. The code needs to compile using C++11 - you are using a few C++14 specific features (e.g. std::decay_t') which we can't support in the current version of cereal.
  2. Ideally we want pull requests done against the develop branch, since there are often other changes staged there before being merged into a stable release (master).
  3. It would be helpful, but it is not required (I'll do it otherwise) if you could add or modify unit tests to reproduce Deserializing ptr to const objects #417.
  4. I'm a bit concerned that the new code creates a temporary shared_ptr before assigning into the wrapper. Perhaps a move assignment would be better here? Previously we were able to create a reference to the actual pointer, but it seems this is not possible if we use a cast.

@AzothAmmo AzothAmmo mentioned this pull request Aug 10, 2017
uint32_t id;

ar( CEREAL_NVP_("id", id) );

if( id & detail::msb_32bit )
{
ptr.reset( detail::Construct<T, Archive>::load_andor_construct() );
auto ptr = std::const_pointer_cast<std::decay_t<T>>(wrapper.ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line appears superfluous to me, @pkrenz ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that ptr is reset in the line right after, so there's no point in assigning it a value, is there?

@pkrenz pkrenz closed this Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants