fmtlib / fmt

A modern formatting library
https://fmt.dev
Other
20.55k stars 2.46k forks source link

Added range_format::string formatter #3973

Closed matt77hias closed 3 months ago

matt77hias commented 4 months ago

Either the basic_string_view range constructor or a user-defined conversion operator will be used.

matt77hias commented 4 months ago

Partially specializing fmt::range_format_kind using range_format::string, enables the correct formatting for all my string-like types. It is unfortunate, however, that I need to list all my string-like types. Ideally, I would like to use something like std::convertible_to< mage::BasicStringView< C > > for C in { char, wchar_t } (+ additional constraints to make it compile :-)), but I keep running in ambiguities with other partial formatter specializations. All of these string-like types are ranges, but none of them is convertible to std::basic_string_view.

template< mage::Character C >
struct range_format_kind< mage::BasicStringView< C >, C >
    : mage::ValueConstant< range_format::string >
{};
matt77hias commented 4 months ago

Note that this can be further extended to support arbitrary ranges. It is a start that makes the string format usable already for some common case.

matt77hias commented 4 months ago

formatter<basic_string<charT>, charT> or formatter<basic_string_view<charT>, charT>? Wouldn't the former construct an intermediate basic_string?

std::basic_string_view's range constructor is more constrained than std::basic_string's std::from_range constructor, but does not require memory allocations.

std::basic_string_view<char> cannot be constructed from std::list<char>. The latter could, however, be formatted without creating an intermediate std::basic_string?

vitaut commented 4 months ago

If you are referring to underlying_ having the type formatter<basic_string<charT>, charT> then it's just for specification. String formatter should behave like formatter<basic_string<charT>, charT> but doesn't have to use it. Although it might be worth constructing intermediate storage at least for non-contiguous ranges.

matt77hias commented 4 months ago

Ok in that case I think it would make sense to use something like

std::conditional_t< std::constructible_from< basic_string_view< Char >, R >,
                    formatter< basic_string_view< Char >, Char >,
                    formatter< basic_string< Char >, Char > >

contiguous_range + sized_range -> basic_string_view (via explicit range constructor) other -> basic_string (via std::from_range constructor)

matt77hias commented 4 months ago

I use std::basic_string_view's C++20 iterator/sentinel constructor instead of std::basic_string_view's C++23 range constructor, and because the former is C++20, I think it would make sense to use <ranges>/__cpp_lib_ranges.

If fmt::basic_string_view gets a corresponding constructor, it would be possible to drop the C++20 constraint. std::basic_string is a bit unfortunate. There is a C++23 range constructor, but prior to that it does not have an iterator/sentinel constructor like std::basic_string_view.

vitaut commented 3 months ago

@matt77hias do you still plan to update this PR to address the comment (remove <ranges> dependency)?

vitaut commented 3 months ago

Merged, thanks!

matt77hias commented 3 months ago

Ah ok now I understand how the ranges header could have been removed entirely. I didn't go that far because fmt's own basic_string_view does not provide the constructor and thus the string_type would always map to std::basic_string in that case. And in order to support that constructor somewhat partially, it would need to include fmt's own ranges header for detail::range_begin and detail::range_end (for use in enable_if), which would require splitting the header or always exposing range formatting as a consequence, because that header would be indirectly included.