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

StringMaker<wchar_t> fail to compile with C++20 enabled (GCC) #357

Closed
lukaszgemborowski opened this issue Apr 23, 2020 · 8 comments
Closed

Comments

@lukaszgemborowski
Copy link
Contributor

lukaszgemborowski commented Apr 23, 2020

Description

StringMaker is using has_insertion_operator trait to check whether type T is accepted by std::ostream operator <<. In C++20 bunch of operator<< overloads were explicitly deleted, see https://en.cppreference.com/w/cpp/io/basic_ostream/operator_ltlt2 causing compilation error.

For me, the alternative form of has_insertion_operator worked well (note this is slightly changed version of an answer found on stackoverflow):

    template<typename T>
    class has_insertion_operator
    {
        template<typename TT>
        static auto test(int)
        -> decltype( std::declval<std::ostream&>() << std::declval<TT>(), std::true_type() );

        template<typename>
        static auto test(...) -> std::false_type;

    public:
        static const bool value = decltype(test<T>(0))::value;
    };

this won't fail to compile and will produce false for deleted overloads of operator <<.

Steps to reproduce

compile with -std=c++20

CHECK_EQ(wchar_t{}, wchar_t{});

Extra information

  • doctest version: latest dev (b2f08ab ) or master
  • Operating System: Ubuntu
  • Compiler+version: g++-10 (Ubuntu 10-20200416-0ubuntu1~18.04) 10.0.1 20200416 (experimental) [master revision 3c3f12e2a76:dcee354ce56:44b326839d864fc10c459916abcc97f35a9ac3de]
@onqtam
Copy link
Member

onqtam commented May 11, 2020

Thanks for reporting this. I'll take a look when I have some time - this is indeed very important.

I wonder if it would be good enough to do a preprocessor comparison check against __cplusplus and have this for C++20 and onwards...

@lukaszgemborowski
Copy link
Contributor Author

lukaszgemborowski commented May 13, 2020

There are two distinct problems here:

  1. compilation issue (which could be easily fixed)
  2. behaviour difference between C++20 and earlier

recap: current code fails to compile (minimal example)
could be fixed with: example which should be valid C++11 (so no need for check against __cplusplus - I assume C++11 is required for doctest anyway) but as you just change the version of C++ to 20 (-std=c++20) the static assertion

static_assert(has_insertion_operator<wchar_t>::value == true);

fails. This leads us to the problem number 2. The question is what are the implications of that difference in behaviour of has_insertion_operator (assuming we just fix the compilation)?

@onqtam
Copy link
Member

onqtam commented May 17, 2020

The implications of just fixing the compile error are that when an assert fails with wchar_t as an argument it won't be properly printed to stdout and the user will instead get {?}. I'm leaning towards just fixing the compilation for now and thinking about the other problem later on.

However, I see that in the fixed code std::declval is used which is from the <utility> header - doctest doesn't include any STL (or C stdlib) headers in the interface part of the header, so that's a problem. If you come up with a different solution I'll be happy to make use of it.

@lukaszgemborowski
Copy link
Contributor Author

I didn't had much time lately but finally I came up with a solution which don't require any additional C++ headers. BTW, my previous example wasn't working correctly and to be honest I don't really know why.

So I did a different approach: https://godbolt.org/z/dyPFTb
There's short implementation at the beginning and then a series of static_asserts serving as kind of compile time unit tests.

@onqtam
Copy link
Member

onqtam commented Jun 11, 2020

I'll probably try this in a branch and see what the CI has to say about it - thanks!

@lukaszgemborowski
Copy link
Contributor Author

No problem. I can open pull request with that change if you like.

@onqtam
Copy link
Member

onqtam commented Jun 11, 2020

sounds good!

@lukaszgemborowski
Copy link
Contributor Author

I believe this issue can be closed now. Thanks for merging the patch! ;-)

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

No branches or pull requests

2 participants