Fix fmt::join for views with input iterators

[Arghnews]

Addresses issue (#3802)

Following from the issue discussion, adding 2 std::moves gets this error:

In file included from /home/justin/code/fmt/test/ranges-test.cc:8:
/home/justin/code/fmt/include/fmt/ranges.h:597:10: error: call to deleted constructor of 'std::ranges::basic_istream_view<int, char>::__iterator'
  597 |     auto it = value.begin;
      |          ^    ~~~~~~~~~~~
/home/justin/code/fmt/include/fmt/base.h:1383:23: note: in instantiation of function template specialization 'fmt::formatter<fmt::join_view<std::ranges::basic_istream_view<int, char>::__iterator, std::default_sentinel_t>>::format<fmt::context>' requested here
 1383 |     ctx.advance_to(cf.format(*static_cast<qualified_type*>(arg), ctx));
      |                       ^
/home/justin/code/fmt/include/fmt/base.h:1364:21: note: in instantiation of function template specialization 'fmt::detail::value<fmt::context>::format_custom_arg<fmt::join_view<std::ranges::basic_istream_view<int, char>::__iterator, std::default_sentinel_t>, fmt::formatter<fmt::join_view<std::ranges::basic_istream_view<int, char>::__iterator, std::default_sentinel_t>>>' requested here
 1364 |     custom.format = format_custom_arg<
      |                     ^
/home/justin/code/fmt/test/ranges-test.cc:634:8: note: in instantiation of function template specialization 'fmt::format<fmt::join_view<std::ranges::basic_istream_view<int, char>::__iterator, std::default_sentinel_t>>' requested here
  634 |   fmt::format("{}", std::move(joined_view)); // Error, use of deleted iterator copy constructor
      |        ^
/usr/lib/llvm-18/bin/../include/c++/v1/__ranges/istream_view.h:72:3: note: '__iterator' has been explicitly marked deleted here
   72 |   __iterator(const __iterator&)                  = delete;
      |   ^

The issue being std::ranges::basic_istream_view::__iterator is not copyable, only movable

The underlying issue is that currently, the joined_view formatter struct formatter<join_view<It, Sentinel, Char>, Char>

has a format func like so:

https://github.com/fmtlib/fmt/blob/810d1750f1579f19d842b6f3ca6671f714375444/include/fmt/ranges.h#L587-L590

Where we take a copy of the iterator which we then mutate by incrementing it which is causing our error

However, we can't mutate the iterator in place as is, as we take the join_view by const& This also prevents us from moving the iterator into it And we can't copy it in case (like this one) it's only movable

If we change the function signature to take it by auto format(join_view<It, Sentinel, Char>& value As mentioned https://github.com/fmtlib/fmt/issues/3802#issuecomment-1889635249 by @tcbrindle This will fix this case, but then break it for other cases where we want the const-ness

---

As I understand it, what we need is a fix that treats these kind of input iterators specially, as they are inherently destructive since they're single pass (probably why the std prevents copying of this one, less foot guns).

We can add a version of our format function that takes value by const ref when not using an input iterator And otherwise just uses a mutable ref to value in place (no copies)

We must disable the const overload for format for input iterators so that

https://github.com/fmtlib/fmt/blob/810d1750f1579f19d842b6f3ca6671f714375444/include/fmt/base.h#L1299-L1302

Correctly deduces that when T's iterator is an input iterator, has_const_formatter_impl returns false And otherwise returns true

---

Since this issue only seems to be a c++20 problem, and no one using c++17 etc has raised it it, I've used concept requires for this. This could be changed to use FMT_ENABLE_IF or std::enable_if_t instead. I've also used the std::input_iterator and std::forward_iterator concepts from <iterator> (already included). The c++17 SFINAE is a bit tricky (and would likely want an ifdef to use the c++20 concepts anyway, as iterator_concept and iterator_category is a bit finnicky etc.)

---

range-v3's istream_view does not have this issue, the iterators are copyable (whether they should be or not is a separate question) Ie. see https://godbolt.org/z/xcb3zf45Y

I've checked the ifdef for the test as best I can Best way I could find was checking __cpp_lib_ranges which means gcc supports this test from gcc >= 12. Clang 15 supports it but it still fails to compile, so clang >= 16 required, and c++20

gcc 11.4 fails cpp_lib_ranges == 202106 gcc 12 works cpp_lib_ranges == 202110

clang 16 works clang 15 still a special sausage with its own error

msvc 19.30 cpp_lib_ranges == 202106 msvc 19.31 cpp_lib_ranges == 202110

---

Sorry for the length of this! Just trying to be clear and lay out my thought process as it's not trivial. Open to feedback. Maybe there's a simpler way to do this that I've missed!

[Arghnews]

Will take another look at this, looks like earlier versions of libc++ etc. didn't have library support for the std::input_iterator concept even in c++20 mode. I had been compiling using llvm-18 with libc++. I hadn't fully appreciated the impact of using a c++20 language feature in requires as well as a c++20 library feature in the std::input_iterator concept

I believe the fix is to disable via ifdefs so this code is only compiled for appropriate c++20 compiler + standard library impl combos and disabled otherwise.

Would this be acceptable?

Also I ran clang-format-18 -i --style=file include/fmt/ranges.h like suggested and still have the formatting diff, unsure why that is

[vitaut]

Would this be acceptable?

I'd recommend using SFINAE instead of conditional compilation if possible.

Also I ran clang-format-18 -i --style=file include/fmt/ranges.h like suggested and still have the formatting diff, unsure why that is

clang-format occasionally produces different results between versions. {fmt} uses 17.0.5 at the moment.

[Arghnews]

Thanks for the feedback, have made the changes you suggested and rebased, hope it's ok now, thanks!

[vitaut]

Merged, thanks!