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

Fix std::variant, std::filesystem::path tests on GCC-8, Clang-7,8. #2951

Merged
merged 2 commits into from
Jul 2, 2022

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jun 28, 2022

libstdc++ from GCC-8 support std::variant.
libstdc++ from GCC-8 has no <version> file.
libstdc++ from GCC-8 defines the macro __cpp_lib_variant in <variant>.

fmt/test/gtest/gtest/gtest.h

Lines 2621 to 2627 in 6a775e9

#ifdef __has_include
#if __has_include(<variant>) && __cplusplus >= 201703L
// Otherwise for C++17 and higher use std::variant for UniversalPrinter<>
// specializations.
#define GTEST_INTERNAL_HAS_VARIANT 1
#include <variant>
namespace testing {

Because gtest.h include <variant>, then the macro __cpp_lib_variant is defined in the test, but is not defined in "fmt/std.h".

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Unfortunately it only masks the issue so I suggest including <variant> in fmt/std.h when the header exists instead. This can be checked with FMT_HAS_INCLUDE.

@phprus phprus marked this pull request as draft June 30, 2022 15:35
@phprus phprus force-pushed the fix-std-1 branch 2 times, most recently from 2581e91 to 1ec4b82 Compare June 30, 2022 16:20
@phprus phprus marked this pull request as ready for review June 30, 2022 16:30
@phprus
Copy link
Contributor Author

phprus commented Jun 30, 2022

@vitaut, Done.
I have added two new CI configurations to detect such errors.

@phprus phprus changed the title Fix std::variant test on GCC-8. Fix std::variant, std::filesystem::path tests on GCC-8, Clang-7,8. Jun 30, 2022
@phprus phprus requested a review from vitaut June 30, 2022 16:58
@@ -17,10 +17,16 @@
#if FMT_HAS_INCLUDE(<version>)
# include <version>
#endif
#if FMT_CPLUSPLUS >= 201703L
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this check for? Shouldn't FMT_HAS_INCLUDE and feature test macro checks be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without FMT_CPLUSPLUS check:
libstdc++: ok (Checking the version of C++ standard is present)
libcxx: ok (Checking the version of C++ standard is absent)
MSVC message:

#if !_HAS_CXX17
#pragma message("The contents of <filesystem> are available only with C++17 or later.")
#else // ^^^ !_HAS_CXX17 / _HAS_CXX17 vvv

https://github.com/microsoft/STL/blob/c34f24920e463a71791c2ee3d2bed14926518965/stl/inc/filesystem#L12-L14
https://github.com/microsoft/STL/blob/c34f24920e463a71791c2ee3d2bed14926518965/stl/inc/variant#L12-L14
https://godbolt.org/z/1MM9Kee8n

For this reason, a check is required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a short comment clarifying that this check is for MSVC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut Done

Comment on lines 81 to 88
try_compile(compile_result_unused
${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_LIST_DIR}/detect-stdfs.cc
CMAKE_FLAGS CMAKE_OSX_ARCHITECTURES=${CMAKE_OSX_ARCHITECTURES}
OUTPUT_VARIABLE RAWOUTPUT)
string(REGEX REPLACE ".*libfound \"([^\"]*)\".*" "\\1" STDLIBFS "${RAWOUTPUT}")
if (STDLIBFS)
target_link_libraries(std-test ${STDLIBFS})
endif ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to simplify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FS-library name is determined by the version of the C++ Standard Library.
CMake only determine the compiler version.
Clang supports libstdc++ and libcxx.
Intel C++ supports libstdc++ on Linux, libcxx on macOS, and Microsoft STL on Windows.
Therefore, the only way to determine which standard library is actually used is by compiling the code.

The code I wrote is the easiest way I know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this information be derived from CMAKE_CXX_STANDARD_LIBRARIES or CMAKE_CXX_IMPLICIT_LINK_LIBRARIES?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable CMAKE_CXX_IMPLICIT_LINK_LIBRARIES allows to detect the standard library name, but not its version.

Example:

GCC-8:
    CMAKE_CXX_IMPLICIT_LINK_LIBRARIES: stdc++;m;gcc_s;gcc;c;gcc_s;gcc

GCC-9:
    CMAKE_CXX_IMPLICIT_LINK_LIBRARIES: stdc++;m;gcc_s;gcc;c;gcc_s;gcc

Clang 13 + libstdc++ (from GCC-11):
    CMAKE_CXX_IMPLICIT_LINK_LIBRARIES: stdc++;m;gcc_s;gcc;c;gcc_s;gcc

Clang 13 + libc++:
    CMAKE_CXX_IMPLICIT_LINK_LIBRARIES: c++;m;gcc_s;gcc;c;gcc_s;gcc

CMAKE_CXX_STANDARD_LIBRARIES - CMake 3.6+ and empty in most configurations

//
// For the license information refer to format.h.

#include <exception>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This include looks unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macros _GLIBCXX_RELEASE and _LIBCPP_VERSION is defined in standard library header files.

<exception> is the smallest of the standard library header files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe include <initializer_list> which is slightly cheaper according to https://artificial-mind.net/projects/compile-health/ and add a comment that this is for _GLIBCXX_RELEASE & _LIBCPP_VERSION?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<initializer_list> is C++11.
Specifying the version of the standard will complicate the call to try_compile.

Comment added.

try_compile(compile_result_unused
${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_LIST_DIR}/detect-stdfs.cc
CMAKE_FLAGS CMAKE_OSX_ARCHITECTURES=${CMAKE_OSX_ARCHITECTURES}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiling detect-stdfs.cc and read compiler output to get fslib info from error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean specifically the above line that forwards CMAKE_OSX_ARCHITECTURES.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMAKE_OSX_ARCHITECTURES affects the compiler but I'm not sure if it affects the standard library as well.
Removed it.

@vitaut vitaut merged commit c12b4c0 into fmtlib:master Jul 2, 2022
@vitaut
Copy link
Contributor

vitaut commented Jul 2, 2022

Merged, thanks!

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

Successfully merging this pull request may close these issues.

2 participants