[libc++][format] Fixes usage of contiguous ranges.

[Mordante]

Summary

The contiguous range made incorrect assumptions for certain input
ranges.

Fixes llvm.org/PR60164

[philnik]

We have iterators of all categories (and sized/unsized sentinels) in test_iterators.h. Why don't you use them?

[Mordante]

Fixes compilation with older compilers.

[JMazurkiewicz]

libcxx/include/__format/range_formatter.h

175

How about using ranges::distance (http://eel.is/c++draft/range.iter.op.distance#4) here instead of ranges::size? We could get rid of ranges::sized_range<_Rp> constraint.

Also, I think we need extra includes: <__ranges/data.h> and <__ranges/size.h> (or <__iterator/distance.h>).

[Mordante]

Thanks for the reviews!

In D142302#4071698, @philnik wrote:

We have iterators of all categories (and sized/unsized sentinels) in test_iterators.h. Why don't you use them?

In the tests for sets and maps @ldionne suggested to use adaptor classes with a minimal interface in the tests. So I thought it would be good to do the same here. I already use some the test iterators, just prior to the new hunk.

libcxx/include/__format/range_formatter.h

175

I expected ranges::distance to require a sized_range, but I didn't check it. But you're right distance would work too and have less requirements.

Indeed these headers should be included. I'm surprised it passes the modular tests without them.

[huixie90]

libcxx/include/__format/range_formatter.h

175

I disagree. Although it is really rare to see a contiguous_range that is not sized_range. But when this happens, I think we do want to constrain it to sized_range, otherwise ranges::distance is going to be a linear operation. so using ranges::size is the right thing

[Mordante]

Adresses review comments.

[Mordante]

Thanks for the feedback!

libcxx/include/__format/range_formatter.h

175

I don't think the linear part will be a big issue; it's still faster than the alternative of copying the range in a temporary buffer. Still I will change it; distance returns a signed value, which runs into narrowing.

(I consider having a contiguous_range that is not a sized_range too rare to add a special case. This entire optimization isn't Standard mandated.)

[Mordante]

Addresses review comments.

libcxx/include/__format/range_formatter.h

struct _LIBCPP_TEMPLATE_VIS _LIBCPP_AVAILABILITY_FORMAT range_formatter {
   }
 
   template <ranges::input_range _Rp, class _FormatContext>
   _LIBCPP_HIDE_FROM_ABI typename _FormatContext::iterator
   __format_as_string(_Rp&& __range, _FormatContext& __ctx, bool __debug_format) const {
     // When the range is contiguous use a basic_string_view instead to avoid a
     // copy of the underlying data. The basic_string_view formatter
     // specialization is the "basic" string formatter in libc++.
-    if constexpr (ranges::contiguous_range<_Rp>) {
+    if constexpr (ranges::contiguous_range<_Rp> && ranges::sized_range<_Rp>) {
       std::formatter<basic_string_view<_CharT>, _CharT> __formatter;
       if (__debug_format)
         __formatter.set_debug_format();
-      return __formatter.format(basic_string_view<_CharT>{__range.data(), __range.size()}, __ctx);
+      return __formatter.format(
+          basic_string_view<_CharT>{
+              ranges::data(__range),
+              ranges::size(__range),
+          },
+          __ctx);
     } else {
       std::formatter<basic_string<_CharT>, _CharT> __formatter;
       if (__debug_format)
         __formatter.set_debug_format();
       // P2106's from_range has not been implemented yet. Instead use a simple
       // copy operation.
       // TODO FMT use basic_string's "from_range" constructor.
       // return std::formatter<basic_string<_CharT>, _CharT>{}.format(basic_string<_CharT>{from_range, __range}, __ctx);

libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.tests.h

 //===----------------------------------------------------------------------===//
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef TEST_STD_UTILITIES_FORMAT_FORMAT_RANGE_FORMAT_RANGE_FORMATTER_FORMAT_FUNCTIONS_TESTS_H
 #define TEST_STD_UTILITIES_FORMAT_FORMAT_RANGE_FORMAT_RANGE_FORMATTER_FORMAT_FUNCTIONS_TESTS_H
 
 #include <algorithm>
 #include <array>
 #include <charconv>
 #include <concepts>
+#include <deque>
 #include <format>
 #include <list>
 #include <ranges>
 #include <span>
 #include <vector>
 
 #include "format.functions.common.h"
 #include "make_string.h"
test_with_ranges<CharT>(
       check, check_exception, std::counted_iterator{bidirectional_iterator<int*>(input.data()), input.size()});
   test_with_ranges<CharT>(
       check, check_exception, std::counted_iterator{random_access_iterator<int*>(input.data()), input.size()});
   test_with_ranges<CharT>(
       check, check_exception, std::counted_iterator{contiguous_iterator<int*>(input.data()), input.size()});
 }
 
 //
+// Adaptor
+//
+
+template <class CharT>
+class non_contiguous {
+  // A deque iterator is random access, but not contiguous.
+  using adaptee = std::deque<CharT>;
+
+public:
+  using iterator = typename adaptee::iterator;
+  using pointer  = typename adaptee::pointer;
+
+  iterator begin() { return data_.begin(); }
+  iterator end() { return data_.end(); }
+
+  explicit non_contiguous(adaptee&& data) : data_(std::move(data)) {}
+
+private:
+  adaptee data_;
+};
+
+template <class CharT>
+class contiguous {
+  // A vector iterator is contiguous.
+  using adaptee = std::vector<CharT>;
+
+public:
+  using iterator = typename adaptee::iterator;
+  using pointer  = typename adaptee::pointer;
+
+  iterator begin() { return data_.begin(); }
+  iterator end() { return data_.end(); }
+
+  explicit contiguous(adaptee&& data) : data_(std::move(data)) {}
+
+private:
+  adaptee data_;
+};
+
+// This tests two different implementations in libc++. A basic_string_view
+// formatter if the range is contiguous, a basic_string otherwise.
+template <class CharT, class TestFunction, class ExceptionTest>
+void test_adaptor(TestFunction check, ExceptionTest check_exception) {
+  static_assert(std::format_kind<non_contiguous<CharT>> == std::range_format::sequence);
+  static_assert(std::ranges::sized_range<non_contiguous<CharT>>);
+  static_assert(!std::ranges::contiguous_range<non_contiguous<CharT>>);
+  test_char_string<CharT>(
+      check,
+      check_exception,
+      non_contiguous<CharT>{std::deque{CharT('H'), CharT('e'), CharT('l'), CharT('l'), CharT('o')}});
+
+  static_assert(std::format_kind<contiguous<CharT>> == std::range_format::sequence);
+  static_assert(std::ranges::sized_range<contiguous<CharT>>);
+  static_assert(std::ranges::contiguous_range<contiguous<CharT>>);
+  test_char_string<CharT>(check,
+                          check_exception,
+                          contiguous<CharT>{std::vector{CharT('H'), CharT('e'), CharT('l'), CharT('l'), CharT('o')}});
+}
+
+//
 // Driver
 //
 
 template <class CharT, class TestFunction, class ExceptionTest>
 void format_tests(TestFunction check, ExceptionTest check_exception) {
   test_char<CharT>(check, check_exception);
 #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
   if (std::same_as<CharT, wchar_t>) // avoid testing twice
     test_char_to_wchar(check, check_exception);
 #endif
   test_bool<CharT>(check, check_exception);
   test_int<CharT>(check, check_exception);
   test_floating_point<CharT>(check, check_exception);
   test_pointer<CharT>(check, check_exception);
   test_string<CharT>(check, check_exception);
 
   test_status<CharT>(check, check_exception); // Has its own handler with its own parser
 
   test_pair_tuple<CharT>(check, check_exception);
   test_tuple_int<CharT>(check, check_exception);
   test_tuple_int_int_int<CharT>(check, check_exception);
 
   test_with_ranges<CharT>(check, check_exception);
+
+  test_adaptor<CharT>(check, check_exception);
 }
 
 #endif // TEST_STD_UTILITIES_FORMAT_FORMAT_RANGE_FORMAT_RANGE_FORMATTER_FORMAT_FUNCTIONS_TESTS_H