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 support for C++17 std::optional and std::variant #448

Closed
wants to merge 2 commits into from

Conversation

arximboldi
Copy link
Contributor

The test and code has been written in similar style to that of the existing support for boost_variant, I hope this is fine. For the tests to run one shall pass -DCMAKE_CXX_STANDARD=17 to cmake.

@patrikhuber
Copy link
Contributor

👍 would love to see this merged as soon as possible! :-)

@arximboldi
Copy link
Contributor Author

Thanks! I'd like to add something in the spirit of #256 for std::variant too but I'd like to know what the maintainers think about it on what the best approach would be...

@AzothAmmo
Copy link
Contributor

This looks good overall - I think some of the files have been misplaced in your commit but I can easily resolve this. The develop branch has also moved to a slightly different form for the unittests where the test and implementation are split amongst hpp/cpp files, but again I can do this easily myself as well.

Regarding #256, which I honestly must have missed, feel free to make a separate PR regarding that kind of change.

@AzothAmmo AzothAmmo added this to the v1.2.3 milestone Oct 30, 2017
@AzothAmmo
Copy link
Contributor

I just tried to fix everything and merge this but it was taking a bit longer than I expected. Here are detailed changes that need to be made:

  1. PR should be against develop branch
  2. test case "optional.cpp" moved to unittests directory
  3. both test cases split into header (tests) and source (instantiation of tests) files, see all other tests for an example
  4. optional and variant are both in the STL, so they should have the doxygen group "STLSupport" and not "OtherTypes"
  5. Missing doxygen comments for functions in optional.hpp

I would also prefer to add a CEREAL_HAS_CPP17 macro to cereal/macros.hpp and use that in place of checking __cplusplus value each time.

I actually did all of these but then got stuck on the next issue:

When we compile with a compiler that does not support C++17, the unit tests that require C++17 should not even be generated. Otherwise what we see in the output is a blank executable that passes all tests, when in actuality there is no test to perform. So we need to add both a list of tests that require C++17 as well as check the C++ version in CMAKE. I got stuck on the latter half of this, so if anyone knows a good way to perform this check (across all operating systems) please let me know.

Also this is a good example of how even an innocuous looking PR can turn into a lot of work!

@patrikhuber
Copy link
Contributor

check the C++ version in CMAKE. I got stuck on the latter half of this, so if anyone knows a good way to perform this check (across all operating systems) please let me know.

I think pybind11 may have a solution for that, I think they detect the available C++ standard (and also allow the user to override it IIRC). You could have a look at their repo and probably the PYBIND11_CPP_STANDARD variable.
Sorry I can't offer a solution directly, but that's where I'd look.

@arximboldi
Copy link
Contributor Author

Wow @AzothAmmo that was a fast and thorough, thanks a lot for fixing all my issues--I take note so next time I hope to be more conforming.

With regard to the open question: could you just use the CMAKE_CXX_STANDARD variable that is passed to CMake? In that case it would be under control of the developer to say what the expected feature set is. Then the unit tests could be organized in subfolders and all tests in the std17 subfolder are skipped if CMAKE_CXX_STANDARD < 17?

There is also this, but this is for core language features, no standard library features: https://cmake.org/cmake/help/v3.1/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html

A last technique is to use the traditional try_compile CMake macro. Basically you pass it a simple program that uses the feature you want to check and see if it works (e.g. a simple program using std::optional<>). Here is more info: https://cmake.org/cmake/help/v3.0/command/try_compile.html

What do you think?

@patrikhuber
Copy link
Contributor

@arximboldi The fine-grained CMAKE_CXX_KNOWN_FEATURES are being phased out according to the cmake developers and shouldn't be used anymore. CMake 3.8 introduced the "meta"-features cxx_std_11, cxx_std_14 and cxx_std_17 to replace the fine-grained ones (https://cmake.org/cmake/help/v3.8/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html).

But yes I think reading the value of CMAKE_CXX_STANDARD, much like pybind11, should suffice in this case and I don't think there's a need for something fine-grained as try_compile. Note though that CMake only started to properly support CMAKE_CXX_STANDARD 17 for the VS generator in 3.10, but I don't think that's a problem, since people using <optional> on VS are very forward-looking and early-adopters anyway (e.g. it needs /std:c++17) and would be on a recent CMake version too.

@AzothAmmo
Copy link
Contributor

CMAKE_CXX_STANDARD looks like the way to go - I was trying to figure this out without it being specified explicitly to CMAKE, but there's really no point in that.

AzothAmmo added a commit that referenced this pull request Nov 6, 2017
Progress towards #448

Lots of warnings from g++7.2, these are addressed in #423 which hasn't been merged yet.
CMAKE can be cleaned up a bit - may put back in the ability to use older cmake and just throw errors as necessary,
otherwise just force people to use at least cmake 3.1
template <class Archive, typename T> inline
void CEREAL_SAVE_FUNCTION_NAME(Archive& ar, const std::optional<T>& optional)
{
if(optional) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the wrong way round. Shouldn't it be if(!optional)? I.e. if the optional is empty, then we store nullopt true, otherwise, we serialise nullopt false plus the content. @arximboldi

@arximboldi
Copy link
Contributor Author

arximboldi commented Nov 11, 2017 via email

@patrikhuber
Copy link
Contributor

patrikhuber commented Nov 11, 2017

Ok, cool!

Hmm, after making the change in both CEREAL_SAVE_FUNCTION_NAME and CEREAL_LOAD_FUNCTION_NAME, I'm still having some issues. The value in the optional seems to get stored to the file (I'm using BinaryArchive), but when loading the same archive, ar(CEREAL_NVP_("nullopt", nullopt)); always returns false. The code is quite simple obviously but I haven't found out yet where it goes wrong.

@patrikhuber
Copy link
Contributor

patrikhuber commented Nov 11, 2017

Forget what I said above, there was a stupid mistake. I shouldn't have negated the check in CEREAL_LOAD_FUNCTION_NAME, because it checks the boolean that's read there, not the actual optional. Only the check in CEREAL_SAVE_FUNCTION_NAME needs to be negated.

All working now :-) 👍

@patrikhuber
Copy link
Contributor

Hi, any updates on completing/merging this? :-O Would love to see it in the official master or develop branch as soon as possible, along with my fix #450.

@arximboldi
Copy link
Contributor Author

Are you talking to me @patrikhuber, is there any follow up I should act on?

@patrikhuber
Copy link
Contributor

@arximboldi I'm not sure actually :-) Maybe @AzothAmmo could let us know whether there's anything we should do?

@patrikhuber
Copy link
Contributor

patrikhuber commented Nov 18, 2017

Hmm actually I seem to get some errors on gcc-7, in cereal/types/variant.hpp:34: https://travis-ci.org/patrikhuber/eos/jobs/304081001#L6855

It compiles fine on clang-5 and VS2017.4 though!

Update: Ignore this comment, it compiles fine on gcc-7, but it errors on clang-5, but it hasn't got anything to do with this PR, it's a clang bug.

@furkanusta
Copy link
Contributor

Are you sure it compiles fine with clang-5? In the line you linked, process starts with

/usr/bin/clang++-5.0 -I/usr/include/opencv ....
but it uses libstdc++ instead of libc++

@patrikhuber
Copy link
Contributor

patrikhuber commented Nov 19, 2017

@furkanusta clang uses libstdc++ by default, so yes it compiles fine with clang-5 in the default configuration, as you can see on the linked travis.
I haven't bothered with testing libc++, as last time I tried (well over a year ago though!), OpenCV didn't compile with it, so it was irrelevant to me.

@furkanusta
Copy link
Contributor

I was referring to line 6855

cd /home/travis/build/patrikhuber/eos/build/examples && /usr/bin/clang++-5.0 -I/usr/include/opencv -I/home/travis/build/patrikhuber/eos/include -I/home/travis/build/patrikhuber/eos/3rdparty/cereal/include -I/home/travis/build/patrikhuber/eos/3rdparty/eigen -I/home/travis/build/patrikhuber/eos/3rdparty/glm -I/home/travis/build/patrikhuber/eos/3rdparty/nanoflann/include -I/home/travis/build/patrikhuber/eos/3rdparty/eigen3-nnls/src -I/home/travis/build/patrikhuber/eos/3rdparty/toml11 -std=c++1z -o CMakeFiles/generate-obj.dir/generate-obj.cpp.o -c /home/travis/build/patrikhuber/eos/examples/generate-obj.cpp

It looks like the failing build is using clang++-5.0, not g++-7

Because there is already a bug report for this issue for clang
https://bugs.llvm.org/show_bug.cgi?id=33222
and also in debian https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=877838

According to debian, it is a clang bug and not because of cereal, though I am not sure if workarounds exist

@patrikhuber
Copy link
Contributor

patrikhuber commented Nov 19, 2017

@furkanusta Oh, you're absolutely correct, apologies. I read it exactly the opposite way. It compiles fine on g++-7, but not on clang-5. And you're correct, this has absolutely nothing to do with this PR, so ignore my comments regarding that.
That's quite an annoying bug actually. I encountered a bug with variant + libstdc++ earlier in the year with clang-4 but it then disappeared when I switched to clang-5, and everything was fine. But apparently it isn't. This will be quite annoying, it probably means it'll be ages before we get to use std::variant on AppleClang/XCode... :-(

@AzothAmmo
Copy link
Contributor

AzothAmmo commented Nov 19, 2017 via email

@AzothAmmo AzothAmmo mentioned this pull request Nov 20, 2017
@AzothAmmo
Copy link
Contributor

Blocked by #423 which fixes some warnings/other things that new compilers will complain about. Almost have that fixed but not quite.

WSoptics pushed a commit to WSoptics/cereal that referenced this pull request Mar 23, 2018
This now correctly saves the optional value, see USCiLab#448.
WSoptics pushed a commit to WSoptics/cereal that referenced this pull request Mar 23, 2018
WSoptics pushed a commit to WSoptics/cereal that referenced this pull request Mar 23, 2018
Also addresses USCiLab#473. A solution that prevents code duplication is the
ultimate goal but this will be fine for now.
WSoptics pushed a commit to WSoptics/cereal that referenced this pull request Mar 23, 2018
WSoptics pushed a commit to WSoptics/cereal that referenced this pull request Mar 23, 2018
WSoptics pushed a commit to WSoptics/cereal that referenced this pull request May 18, 2018
@jonathansharman
Copy link

The optional serializer doesn't seem to work with non-default-constructible types, if even if a load_and_construct function is defined for those types. I don't totally understand the mechanics of load_and_construct, but would it be possible to use it to allow serialization of these types?

jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
Progress towards USCiLab#448

Lots of warnings from g++7.2, these are addressed in USCiLab#423 which hasn't been merged yet.
CMAKE can be cleaned up a bit - may put back in the ability to use older cmake and just throw errors as necessary,
otherwise just force people to use at least cmake 3.1
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
This now correctly saves the optional value, see USCiLab#448.
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
Also addresses USCiLab#473. A solution that prevents code duplication is the
ultimate goal but this will be fine for now.
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
Progress towards USCiLab#448

Lots of warnings from g++7.2, these are addressed in USCiLab#423 which hasn't been merged yet.
CMAKE can be cleaned up a bit - may put back in the ability to use older cmake and just throw errors as necessary,
otherwise just force people to use at least cmake 3.1
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
This now correctly saves the optional value, see USCiLab#448.
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
Also addresses USCiLab#473. A solution that prevents code duplication is the
ultimate goal but this will be fine for now.
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
jrmadsen pushed a commit to jrmadsen/cereal that referenced this pull request Jul 7, 2019
@masariello
Copy link

The optional serializer doesn't seem to work with non-default-constructible types, if even if a load_and_construct function is defined for those types. I don't totally understand the mechanics of load_and_construct, but would it be possible to use it to allow serialization of these types?

Possibly a consequence of #506 ?

@AzothAmmo AzothAmmo moved this from In Progress to Completed in C++17 Features Oct 20, 2019
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
Progress towards USCiLab#448

Lots of warnings from g++7.2, these are addressed in USCiLab#423 which hasn't been merged yet.
CMAKE can be cleaned up a bit - may put back in the ability to use older cmake and just throw errors as necessary,
otherwise just force people to use at least cmake 3.1
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
This now correctly saves the optional value, see USCiLab#448.
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
Also addresses USCiLab#473. A solution that prevents code duplication is the
ultimate goal but this will be fine for now.
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
JerryRyan pushed a commit to JerryRyan/cereal that referenced this pull request Jan 6, 2020
fryguy503 added a commit to wayfarershaven/server that referenced this pull request Jan 11, 2023
Cereal updated for work being done in another branch. This bump fixes some issues with the library. Tested

v1.3.1

Bug fixes and minor enhancements:
Fix typo in docs by @tankorsmash in Fix typo in docs USCiLab/cereal#597
Add MSVC 2019 to build, default ctor for static object by @AzothAmmo in Add MSVC 2019 to build, default ctor for static object USCiLab/cereal#593
Fix json.hpp compilation issue when int32_t is a long by @bblackham in Fix json.hpp compilation issue when int32_t is a long USCiLab/cereal#621
[cpp20] explicitly capture 'this' as copy by @lukaszgemborowski in [cpp20] explicitly capture 'this' as copy USCiLab/cereal#640
Fix rapidjson for Clang 10 by @groscoe2 in Fix rapidjson for Clang 10 USCiLab/cereal#645
Fixes to prevent clang-diagnostic errors by @johngladp in Fixes to prevent clang-diagnostic errors USCiLab/cereal#643
cleanup cmake files to be a little more moderen by @ClausKlein in cleanup cmake files to be a little more moderen USCiLab/cereal#659
GHSA-wgww-fh2f-c855: Store a copy of each serialized shared_ptr within the archive to prevent the shared_ptr to be freed to early. by @serpedon in CVE-2020-11105: Store a copy of each serialized shared_ptr within the archive to prevent the shared_ptr to be freed to early. USCiLab/cereal#667
add license files for components of cereal by @miartad in add license files for components of cereal USCiLab/cereal#676
Catch short documents in JSON input by @johnkeeping in Catch short documents in JSON input USCiLab/cereal#677
C++17: use inline globals for StaticObjects by @InBetweenNames in C++17: use inline globals for StaticObjects USCiLab/cereal#657
Use std::variant::emplace when loading by @kepler-5 in Use std::variant::emplace when loading USCiLab/cereal#699
Use std::optional::emplace() when loading non-empty optional by @kepler-5 in Use std::optional::emplace() when loading non-empty optional USCiLab/cereal#698
Fix itsNextName not clearing when not found + style change by @AzothAmmo in Fix itsNextName not clearing when not found + style change USCiLab/cereal#715
Update doctest to 2.4.6 + local fixes slated for upstream by @AzothAmmo in Update doctest to 2.4.6 + local fixes slated for upstream USCiLab/cereal#716
Fixed loading of std::vector by @Darred in Fixed loading of std::vector<bool> USCiLab/cereal#732
Update license to match BSD template by @AzothAmmo in Update license to match BSD template USCiLab/cereal#735
Update doctest to 2.4.7 by @AzothAmmo in Update doctest to 2.4.7 USCiLab/cereal#736
Use GNUInstallDirs instead of hard wiring install directories by @antonblanchard in Use GNUInstallDirs instead of hard wiring install directories USCiLab/cereal#710
This is not an exhaustive list of changes or individual contributions. See the closed issues or complete changelog for more information.
v1.3.0

New features include:

Deferred serialization for smart pointers (Stack overflow for large chains of shared_ptr (or smart pointers in general) USCiLab/cereal#185)
Initial support for C++17 standard library variant and optional (thanks to @arximboldi, Add serialization support for C++17 std::optional and std::variant USCiLab/cereal#448)
Support for std::atomic (thanks to @bluescarni, Implementation and testing of std::atomic serialization. USCiLab/cereal#277)
Fixes and enhancements include:

Vastly improved continuous integration testing (Appveyor updates + boost testing fixes USCiLab/cereal#568, Update Travis CI USCiLab/cereal#569)
Fixed several issues related to compilation on newer compilers (Fixing various compilation warnings USCiLab/cereal#579, Add fall through comments to json.hpp USCiLab/cereal#587, Fix warning unused private member itsValueItEnd USCiLab/cereal#515)
Fixed warnings with -Wconversion and -Wdocumentation (thanks to @WSoptics, Develop USCiLab/cereal#423)
Performance improvements for polymorphic serialization (PolymorphicVirtualCaster StaticObject instantiation takes a very long time at app startup USCiLab/cereal#354)
Minor fixes and enhancements include:

Fixed a bug related to CEREAL_REGISTER_DYNAMIC_INIT with shared libraries (thanks to @m2tm, Issue correctly using CEREAL_REGISTER_DYNAMIC_INIT USCiLab/cereal#523)
Avoid unnecessary undefined behavior with StaticObject (thanks to @erichkeane, Change StaticObject instance management to hopefully avoid UBSAN USCiLab/cereal#470)
New version.hpp file describes cereal version (detect cereal version at compile time / version.hpp USCiLab/cereal#444)
Ability to disable size=dynamic attribute in the XML archive (thanks to @hoensr, Add option to turn off the size=dynamic attributes of lists USCiLab/cereal#401)
Other Notes
The static checking for minimal serialization has been relaxed as there were many legitimate cases it was interfering with (thanks to @h-2, remove const check from load_minimal USCiLab/cereal#565)
The vs2013 directory has been removed in favor of generating solutions with CMake (Remove vs2013 directory USCiLab/cereal#574)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants