Develop

[WSoptics]

The second commit fixes many compiler warnings that GCC and Clang give when compiling with -Wconversion and -Wdocumentation.

[WSoptics]

The third commit makes sure the specialization for std::vector<bool> is selected even when XMLInputArchive is used, for example, and uses copies in the range-based for loops since that's what std::vector<bool> returns.

[AzothAmmo]

This is in addition to the feedback presented in #418, since these PRs seem to be related. This PR is against the develop branch so you can disregard that comment.

Since one of our targets is MSVC 2013, we can't use noexcept in general. We have a macro defined in cereal/macros.hpp that appropriately replaces noexcept when available, called CEREAL_NOEXCEPT.

[WSoptics]

Thank you for your helpful feedback! I don't understand the last comment, we didn't add any noexcept, did we? Or do you mean we should have at some point?

[WSoptics]

@AzothAmmo I tried removing the unnecessary copies altogether and use std::move when appropriate in 1947a96. I'll rebase when everything is in order.

[WSoptics]

Ah never mind, the noexcept was hidden in rapidxml.hpp :) I fixed it. Let's see what the AppVeyor build now says.

[AzothAmmo]

I'll review this and likely merge either this weekend or early next week, pushing out a small release shortly thereafter with the other develop changes.

[WSoptics]

Awesome, thank you!

[WSoptics]

I added unit tests for shared_ptr<const T>.

Furthermore, I noticed std::unique_ptr<const T> suffered from the same problem. I added both unit tests and a fix for it. However, I believe the case of the user provided load_andor_construct is not tested. Since I don't know what this actually is, I do not feel competent to add a test.

[WSoptics]

OK, one more thing that I noticed: std::decay was wrong here anyway. It would turn a std::shared_ptr<T*> into std::shared_ptr<T>. I fixed it to use std::remove_const, which I believe to be correct. I think now things look good.

[erichkeane]

Are you sure you don't mean std::remove_cv instead of std::remove_const?

[WSoptics]

I am not sure why volatile would have to be removed, but maybe I am missing something.

[erichkeane]

std::decay does 3 things: Removes constant, removes volatile, and removes reference. I'm not sure why it would turn T* into T however...

I haven't looked closely at this component, but I presume that if we meant to remove const, we likely wanted to remove volatile, particularly since it has some pretty nasty performance/allocation gotchas.

[WSoptics]

Sorry, I meant to write T& to T, not the pointer thing.

I am uncertain about volatile. It would seem that a user who uses volatile knows what they're doing and is aware of the adverse performance gotchas. Not sure about allocation issues though.

I'm also uncertain about removing references.

From what I can tell, boost::serialization does not remove references or volatile. However, the code is sufficiently complicated for me to be wrong here.

@erichkeane So maybe you can provide more reasoning as to why remove volatile and references.

[erichkeane]

Ah, that makes more sense...
My biggest concern was that you switched from decay to remove_const in order to lose that T&->T conversion. I feared slightly that you were throwing the baby out with the bathwater:)

Volatile on some platforms requires different alignment requirements. Volatile in a shared_ptr requires a ton more OS loads, and hamstrings the optimizer. Additionally, we would end up generating an entirely new instance of the shared_ptr object in this case.

I'm OK either way, I just want to make sure this is a behavior change being made intentionally.

[AzothAmmo]

Looks good overall and no errors for normal stuff with unique and shared pointers.

Testing with load_and_construct is currently not working, likely due to a type trait problem with const vs non-const.

This reminds me that this also needs to be tested with polymorphic types.

[WSoptics]

@AzothAmmo will you take care of that? I am not sure what to do to be honest.

[AzothAmmo]

Yes I'll take care of that when I merge this.

[AzothAmmo]

Still on my todo list to merge, been very busy lately; should hopefully get this done in the near future (along with anything else marked 1.2.3).

[AzothAmmo]

Looking to merge this as a pre-req for #448. Almost have a solution for load and construct. Test case I was playing with is:

#include <fstream>
#include <iostream>
#include <cereal/cereal.hpp>
#include <cereal/access.hpp>

#include <cereal/archives/json.hpp>
#include <cereal/types/memory.hpp>

struct OneLA
{
  OneLA( int xx ) : x( xx ) {}

  int x;

  template <class Archive>
  void serialize( Archive & ar )
  { ar( x ); }

  template <class Archive, class T>
  static void load_and_construct( Archive & ar, cereal::construct<T> & construct )
  {
    int xx;
    ar( xx );
    construct( xx );
  }

  bool operator==( OneLA const & other ) const
  { return x == other.x; }
};

int main()
{
  std::stringstream ss;

  std::cerr << std::boolalpha;
  std::cerr << cereal::traits::has_member_load_and_construct<OneLA, cereal::JSONOutputArchive>::value << std::endl;
  std::cerr << cereal::traits::has_member_load_and_construct<const OneLA, cereal::JSONOutputArchive>::value << std::endl;

  {
    cereal::JSONOutputArchive ar( ss );
    auto b = std::make_shared<const OneLA>( 3 );
    ar( b );
  }

  std::cout << ss.str() << std::endl;

  {
    std::shared_ptr<const OneLA> b;

    {
      cereal::JSONInputArchive ar(ss);
      ar( b );
    }

    cereal::JSONOutputArchive ar2(std::cout);
    ar2( b );
  }
}

Requires changes in memory

-      std::shared_ptr<typename std::remove_const<T>::type> ptr(reinterpret_cast<T *>(new ST()),
-          [=]( T * t )
+      using NonConstT = typename std::remove_const<T>::type;
+      std::shared_ptr<NonConstT> ptr(reinterpret_cast<NonConstT *>(new ST()),
+          [=]( NonConstT * t )

Need to play with this more and get rid of the extra template parameter I added for load_and_contruct. Ideally we'll just strip away the const internally in all the right places.