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 StringMaker for std::variant, std::monostate #1380

Merged
merged 4 commits into from Sep 20, 2018

Conversation

Projects
None yet
2 participants
@melak47
Contributor

melak47 commented Sep 10, 2018

Description

Adds StringMaker specializations for std::variant and std::monostate, which can be enabled with CATCH_CONFIG_ENABLE_VARIANT_STRINGMAKER or CATCH_CONFIG_ENABLE_ALL_STRINGMAKERS.

Also includes configuration macros for detecting C++17 and header presence, so it doesn't break the build for anyone on C++ < 17 who's using CATCH_CONFIG_ENABLE_ALL_STRINGMAKERS.

Tests are modelled after the tuple tests.

Add StringMaker for std::variant, std::monostate
Add tests and document configuration macros.
@melak47

This comment has been minimized.

Show comment
Hide comment
@melak47

melak47 Sep 10, 2018

Contributor

Hm, seems the tests fail to compile on clang 6 + libstdc++ because of a bug: https://bugs.llvm.org/show_bug.cgi?id=31852
Newer clang should have a fix, newer libstdc++ should have a workaround, but I'm not sure what to do now :/

Edit: clang trunk has the fix, not sure about the upcoming 7.0 release, and gcc 8.2.0's libstdc++has the workaround for clang 6.

Contributor

melak47 commented Sep 10, 2018

Hm, seems the tests fail to compile on clang 6 + libstdc++ because of a bug: https://bugs.llvm.org/show_bug.cgi?id=31852
Newer clang should have a fix, newer libstdc++ should have a workaround, but I'm not sure what to do now :/

Edit: clang trunk has the fix, not sure about the upcoming 7.0 release, and gcc 8.2.0's libstdc++has the workaround for clang 6.

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Sep 13, 2018

Member

You can try modifying .travis.yml to use a newer libstdc++ for the Clang+C++17 builds.

Alternatively, I'll see what I can do on Monday.

Member

horenmar commented Sep 13, 2018

You can try modifying .travis.yml to use a newer libstdc++ for the Clang+C++17 builds.

Alternatively, I'll see what I can do on Monday.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 14, 2018

Codecov Report

Merging #1380 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1380   +/-   ##
=======================================
  Coverage   80.08%   80.08%           
=======================================
  Files         119      119           
  Lines        3389     3389           
=======================================
  Hits         2714     2714           
  Misses        675      675

codecov bot commented Sep 14, 2018

Codecov Report

Merging #1380 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1380   +/-   ##
=======================================
  Coverage   80.08%   80.08%           
=======================================
  Files         119      119           
  Lines        3389     3389           
=======================================
  Hits         2714     2714           
  Misses        675      675
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 14, 2018

Codecov Report

Merging #1380 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
- Coverage   80.11%   80.08%   -0.03%     
==========================================
  Files         119      119              
  Lines        3384     3389       +5     
==========================================
+ Hits         2711     2714       +3     
- Misses        673      675       +2

codecov bot commented Sep 14, 2018

Codecov Report

Merging #1380 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
- Coverage   80.11%   80.08%   -0.03%     
==========================================
  Files         119      119              
  Lines        3384     3389       +5     
==========================================
+ Hits         2711     2714       +3     
- Misses        673      675       +2
@melak47

This comment has been minimized.

Show comment
Hide comment
@melak47

melak47 Sep 14, 2018

Contributor

I tried changing it to libstdc++-8-dev, but that doesn't seem to make a difference.
Also, I just noticed that the output says clang 5.0.0 despite the travis configuration asking for 6.0.

Contributor

melak47 commented Sep 14, 2018

I tried changing it to libstdc++-8-dev, but that doesn't seem to make a difference.
Also, I just noticed that the output says clang 5.0.0 despite the travis configuration asking for 6.0.

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Sep 15, 2018

Member

The output is the image's clang, later on CXX is set to ${COMPILER} which is clang++-6.0 for that image.


Anyway, that sounds like it is time for more compile time configuration, specifically _GLIBCXX_RELEASE(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html).

The basic scheme is to have a check like this

#include <ciso646> // Get libstdc++ and libc++ version macros
#if defined(__CLANG__) && defined(_GLIBCXX_RELEASE) && (CLANG_MAJOR < 7) && (_GLIBCXX_RELEASE < 8)
// This combination of Clang and libstdc++ can't compile std::variant
#define CATCH_INTERNAL_CONFIG_NO_VARIANT
#endif

I don't actually remember Clang's macros, but you get the idea 😃
The "fun" part is that including <ciso646> has observable side effect under MSVC, so it has to be guarded against that as well.

Member

horenmar commented Sep 15, 2018

The output is the image's clang, later on CXX is set to ${COMPILER} which is clang++-6.0 for that image.


Anyway, that sounds like it is time for more compile time configuration, specifically _GLIBCXX_RELEASE(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html).

The basic scheme is to have a check like this

#include <ciso646> // Get libstdc++ and libc++ version macros
#if defined(__CLANG__) && defined(_GLIBCXX_RELEASE) && (CLANG_MAJOR < 7) && (_GLIBCXX_RELEASE < 8)
// This combination of Clang and libstdc++ can't compile std::variant
#define CATCH_INTERNAL_CONFIG_NO_VARIANT
#endif

I don't actually remember Clang's macros, but you get the idea 😃
The "fun" part is that including <ciso646> has observable side effect under MSVC, so it has to be guarded against that as well.

@melak47

This comment has been minimized.

Show comment
Hide comment
@melak47

melak47 Sep 19, 2018

Contributor

I did some more testing, libstdc++ 8.2 has the workaround, 8.1 doesn't, clang trunk appears to have the fix, clang 7 nightly doesn't.
_GLIBCXX_RELEASE only gives the major version, so it ended up begin a little uglier than your sample :)

Contributor

melak47 commented Sep 19, 2018

I did some more testing, libstdc++ 8.2 has the workaround, 8.1 doesn't, clang trunk appears to have the fix, clang 7 nightly doesn't.
_GLIBCXX_RELEASE only gives the major version, so it ended up begin a little uglier than your sample :)

@melak47

This comment has been minimized.

Show comment
Hide comment
@melak47

melak47 Sep 19, 2018

Contributor

Apparently __GLIBCXX__ is useless to detect the actual libstdc++ version, since it can be the datestamp of whenever the snapshot was taken: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html#abi.versioning.__GLIBCXX__

So that leaves disabling the feature for all versions of libstdc++ <= 8, and letting the user override with CATCH_CONFIG_CPP17_VARIANT :/

Contributor

melak47 commented Sep 19, 2018

Apparently __GLIBCXX__ is useless to detect the actual libstdc++ version, since it can be the datestamp of whenever the snapshot was taken: https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html#abi.versioning.__GLIBCXX__

So that leaves disabling the feature for all versions of libstdc++ <= 8, and letting the user override with CATCH_CONFIG_CPP17_VARIANT :/

@horenmar

This comment has been minimized.

Show comment
Hide comment
@horenmar

horenmar Sep 20, 2018

Member

Yeah, __GLIBCXX__ does not work.

I am going to do a quick fixup and then squash + merge the PR into master.

Member

horenmar commented Sep 20, 2018

Yeah, __GLIBCXX__ does not work.

I am going to do a quick fixup and then squash + merge the PR into master.

@horenmar horenmar merged commit c638c57 into catchorg:master Sep 20, 2018

4 checks passed

codecov/patch Coverage not affected when comparing a575536...19a0e10
Details
codecov/project 80.08% remains the same compared to a575536
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@horenmar horenmar added the Tweak label Sep 20, 2018

@melak47 melak47 deleted the melak47:variant-stringmaker branch Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment