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

Merged
merged 4 commits into from
Sep 20, 2018
Merged

Add StringMaker for std::variant, std::monostate #1380

merged 4 commits into from
Sep 20, 2018

Conversation

melak47
Copy link
Contributor

@melak47 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 tests and document configuration macros.
@melak47
Copy link
Contributor Author

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
Copy link
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.

@codecov
Copy link

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
Copy link

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
Copy link
Contributor Author

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
Copy link
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.

@melak47
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
@horenmar horenmar added the Tweak label Sep 20, 2018
@melak47 melak47 deleted the variant-stringmaker branch September 20, 2018 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants