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

Reenable support for fallback formatter to join (#2040) #2050

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

kamibo
Copy link
Contributor

@kamibo kamibo commented Dec 4, 2020

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

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

@@ -147,6 +147,19 @@ struct fallback_formatter<T, Char, enable_if_t<is_streamable<T, Char>::value>>
return std::copy(buffer.begin(), buffer.end(), ctx.out());
}
};

template <typename It, typename Sentinel, typename Char>
struct fallback_formatter<arg_join<It, Sentinel, Char>, Char>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallback_formatter is not an extension point so it shouldn't be specialize for arg_join. Instead make formatter specialization enabled if there is a fallback formatter for the element type as it is done for ranges.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok hope it is better now.

: formatter<typename std::iterator_traits<It>::value_type, Char> {
struct formatter<arg_join<It, Sentinel, Char>, Char> {
using value_type = typename std::iterator_traits<It>::value_type;
using context_type = basic_format_context<arg_join<It, Sentinel, Char>, Char>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed.

using context_type = basic_format_context<arg_join<It, Sentinel, Char>, Char>;
using formatter_type =
conditional_t<has_formatter<value_type, format_context>::value,
typename context_type::template formatter_type<value_type>,
Copy link
Contributor

@vitaut vitaut Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use formatter<value_type, Char> here.

Comment on lines 3724 to 3731
using value_type = typename std::iterator_traits<It>::value_type;
using context_type = basic_format_context<arg_join<It, Sentinel, Char>, Char>;
using formatter_type =
conditional_t<has_formatter<value_type, format_context>::value,
typename context_type::template formatter_type<value_type>,
detail::fallback_formatter<value_type, Char>>;

formatter_type formatting;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be private.

include/fmt/format.h Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one more comment.

@vitaut vitaut merged commit c20874c into fmtlib:master Dec 8, 2020
@vitaut
Copy link
Contributor

vitaut commented Dec 8, 2020

Merged, thanks.

@kamibo
Copy link
Contributor Author

kamibo commented Dec 8, 2020

Thanks for your time and what you have done so far. It is really great!

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.

2 participants