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

Support C++17 features in Catch2 #25236

Closed
davidmatson opened this issue Jun 14, 2022 · 16 comments · Fixed by #25019
Closed

Support C++17 features in Catch2 #25236

davidmatson opened this issue Jun 14, 2022 · 16 comments · Fixed by #25019
Assignees
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist

Comments

@davidmatson
Copy link
Contributor

Does the Catch2 port support compiling with C++17 features enabled?

For example:

inline std::string_view test()
{
    throw std::runtime_error{"bad"};
}

TEST_CASE("Log exception")
{
    REQUIRE(test() == "good");
}

I get a linker error when using catch2 v3.0.1 but not when using v2.*.

Note that Catch2 compiles with C++14 by default.

For more context, see:
catchorg/Catch2#2046
catchorg/Catch2#2210

Is some kind of feature switch needed, or should the port just always use C++17?

@Cheney-W Cheney-W added the category:question This issue is a question label Jun 15, 2022
@Cheney-W
Copy link
Contributor

@davidmatson
Copy link
Contributor Author

What is the recommended solution to provide a port that "just works" for consumers on C++17 or higher? The abseil[cxx17] technique from the linked page?

@Cheney-W
Copy link
Contributor

You could modify the local files manually, add following sentences in the portfile.cmake file.

set(VCPKG_CXX_FLAGS "/std:c++17 ${VCPKG_CXX_FLAGS}")
set(VCPKG_C_FLAGS "/std:c++17 ${VCPKG_C_FLAGS}")

@davidmatson
Copy link
Contributor Author

I'm looking for a way that doesn't require using a port overlay (or patch on top of the vcpkg repo). What's the current best solution in vcpkg? (Perhaps there's a larger vcpkg feature ask here.)

@Cheney-W
Copy link
Contributor

The best way is to let upstream itself set the default language standard to c++17.

@davidmatson
Copy link
Contributor Author

Upstream intentionally supports back to C++14, as documented in their About section here:
https://github.com/catchorg/Catch2

How do other upstream libraries handle supporting older C++ versions?
Suppose a project wants to support compiling with both the latest C++ standard or with older versions, and "just work" either way. Is that possible to do with vcpkg?

@Cheney-W
Copy link
Contributor

Suppose a project wants to support compiling with both the latest C++ standard or with older versions, and "just work" either way. Is that possible to do with vcpkg?

It is their freedom to use that language standard upstream, vcpkg does not make any restrictions on this.
If some users need to use some specific language standards, they can either report their requirements to the upstream, or use the overlay ports feature of vcpkg to customize the ports they need locally.

@davidmatson
Copy link
Contributor Author

davidmatson commented Jun 20, 2022

Does vcpkg support projects that offer compilation with multiple language versions, with a lower minimum but more features on more recent versions (without requiring consumers to use overlay ports)?

@Cheney-W
Copy link
Contributor

If the port itself supports the use of different language standards under different conditions, then vcpkg supports such a port using different conditions as its own feature.

@davidmatson
Copy link
Contributor Author

If the port itself supports the use of different language standards under different conditions, then vcpkg supports such a port using different conditions as its own feature.

In that case, can we add a c++17 feature to the catch2 port? (Or am I misunderstanding your reply?)

@Cheney-W
Copy link
Contributor

Are there explicit conditions in the cmakelists.txt provided by upstrem to control the use of c++17 or c++14?

@davidmatson
Copy link
Contributor Author

Not that I see, though I'm not a CMake expert:
https://github.com/catchorg/Catch2/blob/devel/src/CMakeLists.txt

What would upstream need to do for vcpkg to support a c++17 feature for it?

@Cheney-W
Copy link
Contributor

Upstream clearly mentions c++17 only in https://github.com/catchorg/Catch2/blob/devel/fuzzing/CMakeLists.txt (I only found this part):

# use C++17 so we can get string_view
target_compile_features(fuzzhelper PUBLIC cxx_std_17)

The above part is controlled by the following conditions in https://github.com/catchorg/Catch2/blob/devel/CMakeLists.txt:

if(CATCH_BUILD_FUZZERS)
    add_subdirectory(fuzzing)
endif()

And CATCH_BUILD_FUZZERS is turned off by default. If the content mentioned above is what you said c++17 feature, then we could add CATCH_BUILD_FUZZERS as a feature.

@davidmatson
Copy link
Contributor Author

I think that feature is just about fuzz testing:
cmake-integration.md

One feature I'm interested in is guarded by CATCH_CONFIG_CPP17_STRING_VIEW. There's a C++17 toggle to force it on, but support normally comes via CATCH_INTERNAL_CONFIG_CPP17_STRING_VIEW which is then controlled by CATCH_CPP17_OR_GREATER which is then controlled by normal C++ version macros such as __cplusplus or _MSVC_LANG.

Could we add a feature that controls whether __cplusplus/_MSVC_LANG is C++17, which then means all the C++ 17 features (uncaught_exceptions, string_view, variant, optional, byte) light up?

@Cheney-W
Copy link
Contributor

Could you please submit a PR for this issue?
We can judge whether it conforms to the specification according to the actual modification of the PR.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 22, 2022

There is #25019.
And there is discussion in #24762.

@PhoebeHui PhoebeHui added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist and removed category:question This issue is a question labels Jun 22, 2022
@PhoebeHui PhoebeHui linked a pull request Jun 22, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants