Skip to content

Commit

Permalink
Exclude recursive ranges from the formatter specialization for ranges (
Browse files Browse the repository at this point in the history
…#2974)

* 2954: Add test case

* 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.

* #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

* #2968: Eliminate preprocessor condition that enables the formatter specialization for std::filesystem::path

* #2968: Eliminate preprocessor condition that enables the test for the formatter specialization for std::filesystem::path

* Use own bool_constant, which is available for all C++ versions

* Reintroduce previous workaround but restrict to VS 2015 for now

* Comma fix

* - 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
  • Loading branch information
Dani-Hub authored Jul 10, 2022
1 parent b761f12 commit e1d3d3a
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 23 deletions.
33 changes: 23 additions & 10 deletions include/fmt/ranges.h
Original file line number Diff line number Diff line change
Expand Up @@ -390,22 +390,35 @@ using range_formatter_type = conditional_t<
template <typename R>
using maybe_const_range =
conditional_t<has_const_begin_end<R>::value, const R, R>;

// is_nonrecursive_range depends on fmt::is_range<T, Char>::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 <typename R>
struct is_nonrecursive_range : bool_constant<
!std::is_same<uncvref_type<R>, R>::value> {};

// is_formattable_delayed depends on is_nonrecursive_range<R>::value == true.
// It exists to ensure short-circuit evaluation in the constraint of the
// formatter specialization below.
template <typename R, typename Char>
struct is_formattable_delayed : disjunction<
is_formattable<uncvref_type<maybe_const_range<R>>, Char>,
has_fallback_formatter<uncvref_type<maybe_const_range<R>>, Char>> {};

} // namespace detail

template <typename R, typename Char>
struct formatter<
R, Char,
enable_if_t<
conjunction<fmt::is_range<R, Char>
// Workaround a bug in MSVC 2017 and earlier.
#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
,
disjunction<
is_formattable<detail::uncvref_type<detail::maybe_const_range<R>>,
Char>,
detail::has_fallback_formatter<
detail::uncvref_type<detail::maybe_const_range<R>>, Char>
>
conjunction<fmt::is_range<R, Char>,
detail::is_nonrecursive_range<R>
// Workaround a bug in MSVC 2015 and earlier.
#if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900
,
detail::is_formattable_delayed<R, Char>
#endif
>::value
>> {
Expand Down
5 changes: 0 additions & 5 deletions include/fmt/std.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ inline void write_escaped_path<std::filesystem::path::value_type>(

} // 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 <typename Char>
struct formatter<std::filesystem::path, Char>
: formatter<basic_string_view<Char>> {
Expand All @@ -73,7 +69,6 @@ struct formatter<std::filesystem::path, Char>
basic_string_view<Char>(quoted.data(), quoted.size()), ctx);
}
};
#endif
FMT_END_NAMESPACE
#endif

Expand Down
11 changes: 3 additions & 8 deletions test/std-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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\"");
Expand All @@ -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<std::string>{"abc", "def"};
EXPECT_EQ(fmt::format("path={}, range={}", p, c),
Expand Down

0 comments on commit e1d3d3a

Please sign in to comment.