Skip to content
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

Adding sentinel support to fmt::join(). #1689

Merged
merged 2 commits into from
May 18, 2020
Merged

Conversation

brevzin
Copy link
Contributor

@brevzin brevzin commented May 17, 2020

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

This PR adds support to invoke fmt::join() on ranges that have a sentinel type that differs from its iterator type. This allows you to use fmt::join() on range-v3/C++20 ranges adapters. This shouldn't affect any existing code (whose sentinel is just the same as the iterator type).

@vitaut vitaut merged commit c66aae1 into fmtlib:master May 18, 2020
@vitaut
Copy link
Contributor

vitaut commented May 18, 2020

Thank you!

@puetzk
Copy link
Contributor

puetzk commented Nov 11, 2020

This did technically break any code that instantiated fmt::arg_join<Iterator, char16_t> explicitly. I had one place that was doing this, because it wanted Char = char16_t, but fmt::join is only overloaded for fmt::string_view and fmt::wstring_view, and cannot be partially specialized in order to pull out Char from other options (in this case u16string_view).

The breakage could have been avoided if Sentinel had been introduced as the last template argument, and defaulted, i.e.

template <typename It, typename Char, typename Sentinel = It>
struct arg_join : detail::view {

OTOH, arg_join isn't in c++20 to be compatible with, and changing the template param order to that now would be a breaking change for anyone who has used it its is current form (since this change hit 7.0). Probably not worth trying to change it back...

@vitaut
Copy link
Contributor

vitaut commented Nov 11, 2020

Oops, sorry. It is indeed too late to change it back.

@puetzk
Copy link
Contributor

puetzk commented Nov 12, 2020

Not a problem, I had exactly one use, and it was trivial to fix, and you did label 7 as a major version (so potentially-breaking changes were not a surprise).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants