Skip to content

Commit

Permalink
Fix fmt::join for views with input iterators
Browse files Browse the repository at this point in the history
Addresses issue (fmtlib#3802)

For input_ranges (ie. a range with an input iterator backing it) we
should not copy the iterator and should mutate the iterator on the view.
For forward_ranges (or better), we can keep using them as const and make
a copy of the iterator etc.

This is an issue for code like:
    std::istringstream iss("1 2 3 4 5");
    auto view = std::views::istream<int>(iss)
    fmt::join(std::move(view), ", ")
Since the std::ranges::basic_istream_view::__iterator is not copyable

And the struct formatter<join_view<It, Sentinel, Char>, Char>
only has a
  template <typename FormatContext>
  auto format(const join_view<It, Sentinel, Char>& value,
              FormatContext& ctx) const -> decltype(ctx.out()) {
    auto it = value.begin;
Which takes the value param by const ref and copies the iterator

Fix is disabling the const ref format function overload when we have an
input iterator passed to our join_view, and instead then just mutate the
iterator in place and use a mutable join_view&
  • Loading branch information
Arghnews committed May 1, 2024
1 parent 59f7d3c commit 474b41a
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 2 deletions.
32 changes: 30 additions & 2 deletions include/fmt/ranges.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@
# include <iterator>
# include <tuple>
# include <type_traits>
# include <utility>
#endif

#include "format.h"

#if FMT_CPLUSPLUS >= 202002L
# define FMT_USE_ITERATOR_CONCEPT
#endif

FMT_BEGIN_NAMESPACE

namespace detail {
Expand Down Expand Up @@ -571,7 +576,7 @@ struct join_view : detail::view {
basic_string_view<Char> sep;

join_view(It b, Sentinel e, basic_string_view<Char> s)
: begin(b), end(e), sep(s) {}
: begin(std::move(b)), end(e), sep(s) {}
};

template <typename It, typename Sentinel, typename Char>
Expand All @@ -591,10 +596,33 @@ struct formatter<join_view<It, Sentinel, Char>, Char> {
return value_formatter_.parse(ctx);
}

#ifdef FMT_USE_ITERATOR_CONCEPT
static constexpr bool IsInputIterator =
std::input_iterator<It> && !std::forward_iterator<It>;
#endif

template <typename FormatContext>
#ifdef FMT_USE_ITERATOR_CONCEPT
requires(!IsInputIterator)
#endif
auto format(const join_view<It, Sentinel, Char>& value,
FormatContext& ctx) const -> decltype(ctx.out()) {
auto it = value.begin;
return format_impl(value, ctx, it);
}

#ifdef FMT_USE_ITERATOR_CONCEPT
template <typename FormatContext>
requires IsInputIterator
auto format(join_view<It, Sentinel, Char>& value,
FormatContext& ctx) const -> decltype(ctx.out()) {
return format_impl(value, ctx, value.begin);
}
#endif

template <typename FormatContext>
auto format_impl(const join_view<It, Sentinel, Char>& value,
FormatContext& ctx, It& it) const -> decltype(ctx.out()) {
auto out = ctx.out();
if (it != value.end) {
out = value_formatter_.format(*it, ctx);
Expand All @@ -616,7 +644,7 @@ struct formatter<join_view<It, Sentinel, Char>, Char> {
*/
template <typename It, typename Sentinel>
auto join(It begin, Sentinel end, string_view sep) -> join_view<It, Sentinel> {
return {begin, end, sep};
return {std::move(begin), end, sep};
}

/**
Expand Down
28 changes: 28 additions & 0 deletions test/ranges-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -623,3 +623,31 @@ struct lvalue_qualified_begin_end {
TEST(ranges_test, lvalue_qualified_begin_end) {
EXPECT_EQ(fmt::format("{}", lvalue_qualified_begin_end{}), "[1, 2, 3, 4, 5]");
}

#if !defined(__cpp_lib_ranges) || __cpp_lib_ranges <= 202106L
# define ENABLE_INPUT_RANGE_JOIN_TEST 0
#elif FMT_CLANG_VERSION
# if FMT_CLANG_VERSION > 1500
# define ENABLE_INPUT_RANGE_JOIN_TEST 1
#else
# define ENABLE_INPUT_RANGE_JOIN_TEST 0
# endif
#else
# define ENABLE_INPUT_RANGE_JOIN_TEST 1
#endif

#if ENABLE_INPUT_RANGE_JOIN_TEST
TEST(ranges_test, input_range_join) {
std::istringstream iss("1 2 3 4 5");
auto view = std::views::istream<std::string>(iss);
auto joined_view = fmt::join(view.begin(), view.end(), ", ");
EXPECT_EQ("1, 2, 3, 4, 5", fmt::format("{}", std::move(joined_view)));
}

TEST(ranges_test, input_range_join_overload) {
std::istringstream iss("1 2 3 4 5");
EXPECT_EQ(
"1.2.3.4.5",
fmt::format("{}", fmt::join(std::views::istream<std::string>(iss), ".")));
}
#endif

0 comments on commit 474b41a

Please sign in to comment.