From d2473b7b73c0af2a3ed34c99e50ace0a1040581a Mon Sep 17 00:00:00 2001
From: Victor Zverovich <viz@meta.com>
Date: Wed, 8 May 2024 15:19:19 -0700
Subject: [PATCH] Simplify join_view formatter

---
 include/fmt/ranges.h | 38 +++++++++++++-------------------------
 test/ranges-test.cc  | 42 +++++++++++++++++++++---------------------
 2 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h
index 573f2e7b4aff..ed203ae7b299 100644
--- a/include/fmt/ranges.h
+++ b/include/fmt/ranges.h
@@ -623,41 +623,29 @@ struct formatter<join_view<It, Sentinel, Char>, Char> {
 #endif
   formatter<remove_cvref_t<value_type>, Char> value_formatter_;
 
+  using view_ref = conditional_t<std::is_copy_constructible<It>::value,
+                                 const join_view<It, Sentinel, Char>&,
+                                 join_view<It, Sentinel, Char>&&>;
+
  public:
   template <typename ParseContext>
   FMT_CONSTEXPR auto parse(ParseContext& ctx) -> const Char* {
     return value_formatter_.parse(ctx);
   }
 
-  template <typename FormatContext, typename Iter,
-            FMT_ENABLE_IF(std::is_copy_constructible<Iter>::value)>
-  auto format(const join_view<Iter, Sentinel, Char>& value,
-              FormatContext& ctx) const -> decltype(ctx.out()) {
-    auto it = value.begin;
-    return do_format(value, ctx, it);
-  }
-
-  template <typename FormatContext, typename Iter,
-            FMT_ENABLE_IF(!std::is_copy_constructible<Iter>::value)>
-  auto format(join_view<Iter, Sentinel, Char>& value, FormatContext& ctx) const
-      -> decltype(ctx.out()) {
-    return do_format(value, ctx, value.begin);
-  }
-
- private:
   template <typename FormatContext>
-  auto do_format(const join_view<It, Sentinel, Char>& value, FormatContext& ctx,
-                 It& it) const -> decltype(ctx.out()) {
+  auto format(view_ref& value, FormatContext& ctx) const
+      -> decltype(ctx.out()) {
+    auto it = std::forward<view_ref>(value).begin;
     auto out = ctx.out();
-    if (it != value.end) {
+    if (it == value.end) return out;
+    out = value_formatter_.format(*it, ctx);
+    ++it;
+    while (it != value.end) {
+      out = detail::copy<Char>(value.sep.begin(), value.sep.end(), out);
+      ctx.advance_to(out);
       out = value_formatter_.format(*it, ctx);
       ++it;
-      while (it != value.end) {
-        out = detail::copy<Char>(value.sep.begin(), value.sep.end(), out);
-        ctx.advance_to(out);
-        out = value_formatter_.format(*it, ctx);
-        ++it;
-      }
     }
     return out;
   }
diff --git a/test/ranges-test.cc b/test/ranges-test.cc
index 67bf44f9be37..b2c0b67ad5c1 100644
--- a/test/ranges-test.cc
+++ b/test/ranges-test.cc
@@ -667,14 +667,14 @@ TEST(ranges_test, lvalue_qualified_begin_end) {
 
 #if ENABLE_INPUT_RANGE_JOIN_TEST
 TEST(ranges_test, input_range_join) {
-  std::istringstream iss("1 2 3 4 5");
+  auto iss = std::istringstream("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)));
+  EXPECT_EQ("1, 2, 3, 4, 5",
+            fmt::format("{}", fmt::join(view.begin(), view.end(), ", ")));
 }
 
 TEST(ranges_test, input_range_join_overload) {
-  std::istringstream iss("1 2 3 4 5");
+  auto iss = std::istringstream("1 2 3 4 5");
   EXPECT_EQ(
       "1.2.3.4.5",
       fmt::format("{}", fmt::join(std::views::istream<std::string>(iss), ".")));
@@ -682,26 +682,26 @@ TEST(ranges_test, input_range_join_overload) {
 #endif
 
 TEST(ranges_test, std_istream_iterator_join) {
-  std::istringstream iss("1 2 3 4 5");
-  std::istream_iterator<int> first{iss};
-  std::istream_iterator<int> last{};
-  auto joined_view = fmt::join(first, last, ", ");
-  EXPECT_EQ("1, 2, 3, 4, 5", fmt::format("{}", std::move(joined_view)));
+  auto&& iss = std::istringstream("1 2 3 4 5");
+  auto first = std::istream_iterator<int>(iss);
+  auto last = std::istream_iterator<int>();
+  EXPECT_EQ("1, 2, 3, 4, 5", fmt::format("{}", fmt::join(first, last, ", ")));
 }
 
 TEST(ranges_test, movable_only_istream_iter_join) {
-  // Mirrors c++20 std::ranges::basic_istream_view::iterator
-  struct UncopyableIstreamIter : std::istream_iterator<int> {
-    explicit UncopyableIstreamIter(std::istringstream& iss)
+  // Mirrors C++20 std::ranges::basic_istream_view::iterator.
+  struct noncopyable_istream_iterator : std::istream_iterator<int> {
+    explicit noncopyable_istream_iterator(std::istringstream& iss)
         : std::istream_iterator<int>{iss} {}
-    UncopyableIstreamIter(const UncopyableIstreamIter&) = delete;
-    UncopyableIstreamIter(UncopyableIstreamIter&&) = default;
+    noncopyable_istream_iterator(const noncopyable_istream_iterator&) = delete;
+    noncopyable_istream_iterator(noncopyable_istream_iterator&&) = default;
   };
-  static_assert(!std::is_copy_constructible<UncopyableIstreamIter>::value, "");
-
-  std::istringstream iss("1 2 3 4 5");
-  UncopyableIstreamIter first{iss};
-  std::istream_iterator<int> last{};
-  auto joined_view = fmt::join(std::move(first), last, ", ");
-  EXPECT_EQ("1, 2, 3, 4, 5", fmt::format("{}", std::move(joined_view)));
+  static_assert(
+      !std::is_copy_constructible<noncopyable_istream_iterator>::value, "");
+
+  auto&& iss = std::istringstream("1 2 3 4 5");
+  auto first = noncopyable_istream_iterator(iss);
+  auto last = std::istream_iterator<int>();
+  EXPECT_EQ("1, 2, 3, 4, 5",
+            fmt::format("{}", fmt::join(std::move(first), last, ", ")));
 }