Add StringMaker for std::string_view

[melak47]

Description

Adds StringMaker specializations for std::string_view and std::wstring_view,
so that string_views gets stringified correctly.

GitHub Issues

#1375

[codecov]

Codecov Report

Merging #1376 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1376   +/-   ##
=======================================
  Coverage   80.11%   80.11%           
=======================================
  Files         119      119           
  Lines        3384     3384           
=======================================
  Hits         2711     2711           
  Misses        673      673

[horenmar]

The implementation itself looks good, even if the conversion is needlessly inefficient by allocating a new string, but there are no tests.

The tests will also need to be conditionally compiled, and they will need to be removed from the approval testing by giving them [approvals] tag. (See e.g. "Eating cucumbers" scenario in projects/SelfTest/UsageTests/Generators.tests.cpp, but it doesn't need to be hidden.)

[horenmar]

This belongs to catch_compiler_capabilities.h and should work a bit differently (there are other similar macros there already, you can use them as a rough template).

[melak47]

Work differently how?
Using _MSCVC_LANG is necessary to detect whether the user is compiling with e.g. /std:c++17 on msvc, and is going to be necessary at least until the next major release .
Or do you mean putting this logic into some CATCH_INTERNAL_MSVC_CPP17_OR_GREATER macros?

[melak47]

Is this more like what you'd expect (in catch_compiler_capabilities.h)?

#  if _MSC_VER >= 1910
#    if defined(_MSVC_LANG)
#      if _MSVC_LANG >= 201402L
#        define CATCH_INTERNAL_CONFIG_MSVC_CPP14_OR_GREATER
#      elif _MSVC_LANG >= 201703L
#        define CATCH_INTERNAL_CONFIG_MSVC_CPP17_OR_GREATER
#      endif
#    endif
#  endif

// ...

////////////////////////////////////////////////////////////////////////////////
// Check if string_view is available and usable
#if defined(__has_include) && __has_include(<string_view>) && (CATCH_CPP17_OR_GREATER || CATCH_INTERNAL_CONFIG_MSVC_CPP17_OR_GREATER)
#    define CATCH_INTERNAL_CONFIG_STRING_VIEW
#endif

// ...

#if defined(CATCH_INTERNAL_CONFIG_STRING_VIEW) && !defined(CATCH_CONFIG_NO_STRING_VIEW) && !defined(CATCH_CONFIG_STRING_VIEW)
#  define CATCH_CONFIG_STRING_VIEW
#endif

[horenmar]

Yeah, this is much better -- in our experience, the autodetection will always break down on some obscure platform, so feature toggles need to let the user override them as they see fit, which is why there is the INTERNAL_* and NO_* distinction in defines.

2 more things:

1. CATCH_CONFIG_STRING_VIEW should be CATCH_CONFIG_CPP17_STRING_VIEW (respectively CATCH_INTERNAL_CONFIG_CPP17_STRING_VIEW)
2. Is there an actual need for MSVC-specific C++ standard defines? (e.g. cannot it just toggle CATCH_CPP14_OR_GREATER and CATCH_CPP17_OR_GREATER?)

[melak47]

__cplusplus is still 199711L on MSVC, and _MSVC_LANG matches what standard you selected with compiler flags.
With recent versions, you can also use /Zc:__cplusplus to make __cplusplus match _MSVC_LANG, but this will stay disabled by default until at least the next major release of MSVC. See https://blogs.msdn.microsoft.com/vcblog/2018/04/09/msvc-now-correctly-reports-__cplusplus/.

[horenmar]

I would move one of the two specializations so that there is less conditionally compiled blocks of code and things are easier to read, but that is a minor nitpick.

(This also applies to the implementation in the .cpp file)

[melak47]

The std::wstring specialization should only be present when CATCH_CONFIG_WCHAR is defined,
std::string_view only when string_view support is on, std::wstring_view only when both are on - I could move blocks around so the ifdefs don't nest, but wouldn't it still take three ifdef'ed blocks?

Edit: Ok, you're right, I see some blocks that can be merged in the .cpp file.

[horenmar]

Thanks for the PR, I added some comments on changes that need to be done before it can be merged.

[melak47]

I'm happy to add tests, but there seem to be no tests that specifically check whether std::string and const char* are stringified the same, so I'm not entirely sure what you'd want the tests for std::string_view to look like.
There are some tests for stringifying containers of strings, where the quoting behavior is indirectly tested for std::string.

Do you think something simple like

TEST_CASE("String views are stringified like other strings", "[toString][approvals]") {
    std::string_view view{"abc"};
    CHECK(Catch::Detail::stringify(view) == R"("abc")");

    std::string_view arr[] { view };
    CHECK(Catch::Detail::stringify(arr) == R"({ "abc" })");
}

is enough? If not, could you elaborate on what you're looking for?

---

As for the inefficient implementation, the char*/const char*/ wchar_t*/ const wchar_t* specializations are implemented the same way.

[horenmar]

Yep, those tests are enough.

I remembered that the PR will need one more thing though, documentation. The define needs to be added to docs/configuration.md, specifically the "C++17 toggles part". Just add something like

CATCH_CONFIG_CPP17_STRING_VIEW // Provide StringMaker specialization for std::string_view

---

As for the inefficient implementation, the char*/const char*/ wchar_t*/ const wchar_t* specializations are implemented the same way.

Yeah, that's why it is a minor nitpick in a comment, rather than part of the review 😃 At some point I will need to rewrite the stringification implementations -- it is causing performance problems when there is a lot of output to be written.

[horenmar]

I fixed the issue on older compilers and squashed down the PR before merge.