-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix fmt::join for views with input iterators #3946
Conversation
Will take another look at this, looks like earlier versions of libc++ etc. didn't have library support for the 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 |
I'd recommend using SFINAE instead of conditional compilation if possible.
clang-format occasionally produces different results between versions. {fmt} uses 17.0.5 at the moment. |
include/fmt/ranges.h
Outdated
template <typename FormatContext> | ||
#ifdef FMT_USE_ITERATOR_CONCEPT | ||
requires(!is_input_iterator) | ||
#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 is_input_iterator | ||
auto format(join_view<It, Sentinel, Char>& value, | ||
FormatContext& ctx) const -> decltype(ctx.out()) { | ||
return format_impl(value, ctx, value.begin); | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should (1) check if the iterator is copyable rather than trying to detect if it is an input iterator and (2) use SFINAE instead of concepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion, certainly simpler, have made the change
Other changes introduced are just formatting from running clang-format (17.0.5 as you suggested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good but please rebase and address the minor comment inline.
913b61a
to
7013466
Compare
If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (fmtlib#3802)
7013466
to
9e0ba96
Compare
Thanks for the feedback, have made the changes you suggested and rebased, hope it's ok now, thanks! |
Merged, thanks! |
Addresses issue (#3802)
Following from the issue discussion, adding 2 std::moves gets this error:
The issue being
std::ranges::basic_istream_view::__iterator
is not copyable, only movableThe underlying issue is that currently, the joined_view formatter
struct formatter<join_view<It, Sentinel, Char>, Char>
has a format func like so:
fmt/include/fmt/ranges.h
Lines 587 to 590 in 810d175
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 #3802 (comment) 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 iteratorAnd 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
fmt/include/fmt/base.h
Lines 1299 to 1302 in 810d175
Correctly deduces that when T's iterator is an input iterator,
has_const_formatter_impl
returns falseAnd 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
orstd::enable_if_t
instead.I've also used the
std::input_iterator
andstd::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!