From d51148599a9ea65bea335204f5595f73209023c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Sat, 2 Jul 2022 19:04:40 +0200 Subject: [PATCH 1/9] 2954: Add test case --- test/CMakeLists.txt | 2 ++ test/ranges-std-test.cc | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 test/ranges-std-test.cc diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 5ac1962916c5..dda5665601d7 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -78,6 +78,7 @@ add_fmt_test(printf-test) add_fmt_test(ranges-test ranges-odr-test.cc) add_fmt_test(scan-test) add_fmt_test(std-test) +add_fmt_test(ranges-std-test) try_compile(compile_result_unused ${CMAKE_CURRENT_BINARY_DIR} SOURCES ${CMAKE_CURRENT_LIST_DIR}/detect-stdfs.cc @@ -85,6 +86,7 @@ try_compile(compile_result_unused string(REGEX REPLACE ".*libfound \"([^\"]*)\".*" "\\1" STDLIBFS "${RAWOUTPUT}") if (STDLIBFS) target_link_libraries(std-test ${STDLIBFS}) + target_link_libraries(ranges-std-test ${STDLIBFS}) endif () add_fmt_test(unicode-test HEADER_ONLY) if (MSVC) diff --git a/test/ranges-std-test.cc b/test/ranges-std-test.cc new file mode 100644 index 000000000000..6c8e8e4b05d6 --- /dev/null +++ b/test/ranges-std-test.cc @@ -0,0 +1,26 @@ +// Formatting library for C++ - tests for ranges and std combination +// +// Copyright (c) 2012 - present, Victor Zverovich +// All rights reserved. +// +// For the license information refer to format.h. +// +// Copyright (c) 2022 - present, Dani-Hub (Daniel Kruegler) +// All rights reserved + +#include "fmt/ranges.h" +#include "fmt/std.h" + +#include +#include + +#include "gtest/gtest.h" + +TEST(ranges_std_test, format_vector_path) { +#ifdef __cpp_lib_filesystem + auto p = std::filesystem::path("foo/bar.txt"); + auto c = std::vector{"abc", "def"}; + EXPECT_EQ(fmt::format("path={}, range={}", p, c), + "path=\"foo/bar.txt\", range=[\"abc\", \"def\"]"); +#endif +} From 6d145740875543e0ac5527710395c7bd23ec574f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Sun, 3 Jul 2022 18:25:34 +0200 Subject: [PATCH 2/9] Eliminate extra-test and merge it into existing std-test instead. Add conditionals for filesystem::path testing that does not run into the ambiguity problem. --- test/CMakeLists.txt | 2 -- test/ranges-std-test.cc | 26 -------------------------- 2 files changed, 28 deletions(-) delete mode 100644 test/ranges-std-test.cc diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index dda5665601d7..5ac1962916c5 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -78,7 +78,6 @@ add_fmt_test(printf-test) add_fmt_test(ranges-test ranges-odr-test.cc) add_fmt_test(scan-test) add_fmt_test(std-test) -add_fmt_test(ranges-std-test) try_compile(compile_result_unused ${CMAKE_CURRENT_BINARY_DIR} SOURCES ${CMAKE_CURRENT_LIST_DIR}/detect-stdfs.cc @@ -86,7 +85,6 @@ try_compile(compile_result_unused string(REGEX REPLACE ".*libfound \"([^\"]*)\".*" "\\1" STDLIBFS "${RAWOUTPUT}") if (STDLIBFS) target_link_libraries(std-test ${STDLIBFS}) - target_link_libraries(ranges-std-test ${STDLIBFS}) endif () add_fmt_test(unicode-test HEADER_ONLY) if (MSVC) diff --git a/test/ranges-std-test.cc b/test/ranges-std-test.cc deleted file mode 100644 index 6c8e8e4b05d6..000000000000 --- a/test/ranges-std-test.cc +++ /dev/null @@ -1,26 +0,0 @@ -// Formatting library for C++ - tests for ranges and std combination -// -// Copyright (c) 2012 - present, Victor Zverovich -// All rights reserved. -// -// For the license information refer to format.h. -// -// Copyright (c) 2022 - present, Dani-Hub (Daniel Kruegler) -// All rights reserved - -#include "fmt/ranges.h" -#include "fmt/std.h" - -#include -#include - -#include "gtest/gtest.h" - -TEST(ranges_std_test, format_vector_path) { -#ifdef __cpp_lib_filesystem - auto p = std::filesystem::path("foo/bar.txt"); - auto c = std::vector{"abc", "def"}; - EXPECT_EQ(fmt::format("path={}, range={}", p, c), - "path=\"foo/bar.txt\", range=[\"abc\", \"def\"]"); -#endif -} From 26a28441d3aca9e412db7062d7bdfb5c4be2984d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 19:53:40 +0200 Subject: [PATCH 3/9] #2968: Introduce additional compile-time predicate to detect recursive ranges and reject them in formatter specialization for ranges. In addition, introduce additional wrapper traits for the individual logical operands of the complete range constraints --- include/fmt/ranges.h | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h index 10429fc8eda3..0010f49e7e88 100644 --- a/include/fmt/ranges.h +++ b/include/fmt/ranges.h @@ -390,23 +390,33 @@ using range_formatter_type = conditional_t< template using maybe_const_range = conditional_t::value, const R, R>; + +template +struct is_not_recursive_range : std::bool_constant< + !std::is_same, R>::value> {}; + +template +struct is_formattable_delayed : is_formattable< + detail::uncvref_type>, Char> { +}; + +template +struct has_fallback_formatter_delayed : detail::has_fallback_formatter< + detail::uncvref_type>, Char> { +}; + } // namespace detail template struct formatter< R, Char, enable_if_t< - conjunction -// Workaround a bug in MSVC 2017 and earlier. -#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920 - , - disjunction< - is_formattable>, - Char>, - detail::has_fallback_formatter< - detail::uncvref_type>, Char> - > -#endif + conjunction, + detail::is_not_recursive_range, + disjunction< + detail::is_formattable_delayed, + detail::has_fallback_formatter_delayed + > >::value >> { From c6f372bb38e356bea0c22ed69c9e28fd06681910 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 19:55:19 +0200 Subject: [PATCH 4/9] #2968: Eliminate preprocessor condition that enables the formatter specialization for std::filesystem::path --- include/fmt/std.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/include/fmt/std.h b/include/fmt/std.h index 227f4841123d..41d2b2838b6d 100644 --- a/include/fmt/std.h +++ b/include/fmt/std.h @@ -57,10 +57,6 @@ inline void write_escaped_path( } // namespace detail -#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920 -// For MSVC 2017 and earlier using the partial specialization -// would cause an ambiguity error, therefore we provide it only -// conditionally. template struct formatter : formatter> { @@ -73,7 +69,6 @@ struct formatter basic_string_view(quoted.data(), quoted.size()), ctx); } }; -#endif FMT_END_NAMESPACE #endif From a1ec9fc771650ef4671452982f72b0ab58ed5895 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 19:57:20 +0200 Subject: [PATCH 5/9] #2968: Eliminate preprocessor condition that enables the test for the formatter specialization for std::filesystem::path --- test/std-test.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/test/std-test.cc b/test/std-test.cc index fc8f72a0f0fb..c22b3e38fb39 100644 --- a/test/std-test.cc +++ b/test/std-test.cc @@ -14,10 +14,7 @@ #include "gtest/gtest.h" TEST(std_test, path) { -// Test ambiguity problem described in #2954. We need to exclude compilers -// where the ambiguity problem cannot be solved for now. -#if defined(__cpp_lib_filesystem) && \ - (!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920) +#ifdef __cpp_lib_filesystem EXPECT_EQ(fmt::format("{:8}", std::filesystem::path("foo")), "\"foo\" "); EXPECT_EQ(fmt::format("{}", std::filesystem::path("foo\"bar.txt")), "\"foo\\\"bar.txt\""); @@ -37,10 +34,8 @@ TEST(std_test, path) { } TEST(ranges_std_test, format_vector_path) { -// Test ambiguity problem described in #2954. We need to exclude compilers -// where the ambiguity problem cannot be solved for now. -#if defined(__cpp_lib_filesystem) && \ - (!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920) +// Test ambiguity problem described in #2954. +#ifdef __cpp_lib_filesystem auto p = std::filesystem::path("foo/bar.txt"); auto c = std::vector{"abc", "def"}; EXPECT_EQ(fmt::format("path={}, range={}", p, c), From e4c2e72a183a52989b46c8c0261f77dc1aec62e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 20:02:10 +0200 Subject: [PATCH 6/9] Use own bool_constant, which is available for all C++ versions --- include/fmt/ranges.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h index 0010f49e7e88..e138ff55ee9f 100644 --- a/include/fmt/ranges.h +++ b/include/fmt/ranges.h @@ -392,7 +392,7 @@ using maybe_const_range = conditional_t::value, const R, R>; template -struct is_not_recursive_range : std::bool_constant< +struct is_not_recursive_range : bool_constant< !std::is_same, R>::value> {}; template From 7b05da767bd8faeb5ef0f4ad2e9e836cf69d4c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 20:22:47 +0200 Subject: [PATCH 7/9] Reintroduce previous workaround but restrict to VS 2015 for now --- include/fmt/ranges.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h index e138ff55ee9f..d06de6a2f823 100644 --- a/include/fmt/ranges.h +++ b/include/fmt/ranges.h @@ -413,10 +413,13 @@ struct formatter< enable_if_t< conjunction, detail::is_not_recursive_range, +// Workaround a bug in MSVC 2015 and earlier. +#if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900 disjunction< detail::is_formattable_delayed, detail::has_fallback_formatter_delayed > +#endif >::value >> { From bfff7ed4b75fc56d00144a614b0b84e4c461bbc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Wed, 6 Jul 2022 20:32:19 +0200 Subject: [PATCH 8/9] Comma fix --- include/fmt/ranges.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h index d06de6a2f823..280d1c57b886 100644 --- a/include/fmt/ranges.h +++ b/include/fmt/ranges.h @@ -412,9 +412,10 @@ struct formatter< R, Char, enable_if_t< conjunction, - detail::is_not_recursive_range, + detail::is_not_recursive_range // Workaround a bug in MSVC 2015 and earlier. #if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900 + , disjunction< detail::is_formattable_delayed, detail::has_fallback_formatter_delayed From c03288b0a7ee49b4753c8e45b090df916d9b75a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= Date: Sun, 10 Jul 2022 09:16:17 +0200 Subject: [PATCH 9/9] - Rename is_not_recursive_range to is_nonrecursive_range and add comment that explains it being depending on is_range being true - Merge has_fallback_formatter_delayed into is_formattable_delayed and add comment that explains it being depending on is_not_recursive_range being true - Replace disjunction in formatter specialization by has_fallback_formatter_delayed - Get rid of unneeded detail:: prefixes within namespace detail --- include/fmt/ranges.h | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h index 280d1c57b886..8b7c9aad27e1 100644 --- a/include/fmt/ranges.h +++ b/include/fmt/ranges.h @@ -391,19 +391,21 @@ template using maybe_const_range = conditional_t::value, const R, R>; +// is_nonrecursive_range depends on fmt::is_range::value == true. +// It exists to ensure short-circuit evaluation in the constraint of the +// formatter specialization below. A similar approach is used in +// https://wg21.link/p2286. template -struct is_not_recursive_range : bool_constant< - !std::is_same, R>::value> {}; +struct is_nonrecursive_range : bool_constant< + !std::is_same, R>::value> {}; +// is_formattable_delayed depends on is_nonrecursive_range::value == true. +// It exists to ensure short-circuit evaluation in the constraint of the +// formatter specialization below. template -struct is_formattable_delayed : is_formattable< - detail::uncvref_type>, Char> { -}; - -template -struct has_fallback_formatter_delayed : detail::has_fallback_formatter< - detail::uncvref_type>, Char> { -}; +struct is_formattable_delayed : disjunction< + is_formattable>, Char>, + has_fallback_formatter>, Char>> {}; } // namespace detail @@ -412,14 +414,11 @@ struct formatter< R, Char, enable_if_t< conjunction, - detail::is_not_recursive_range + detail::is_nonrecursive_range // Workaround a bug in MSVC 2015 and earlier. #if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900 , - disjunction< - detail::is_formattable_delayed, - detail::has_fallback_formatter_delayed - > + detail::is_formattable_delayed #endif >::value >> {