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

Added range_format::string formatter #3973

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

matt77hias
Copy link
Contributor

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

@matt77hias
Copy link
Contributor Author

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 matt77hias force-pushed the range_format_string branch 4 times, most recently from e6557dd to 4684c03 Compare May 23, 2024 18:46
@matt77hias
Copy link
Contributor Author

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.

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! Please add a test case to ranges-test and address comments.

include/fmt/ranges.h Outdated Show resolved Hide resolved
include/fmt/ranges.h Outdated Show resolved Hide resolved
@matt77hias matt77hias force-pushed the range_format_string branch 3 times, most recently from 2939345 to 902e4de Compare May 26, 2024 17:56
@@ -445,7 +445,7 @@ struct range_formatter<
FMT_CONSTEXPR auto parse(ParseContext& ctx) -> decltype(ctx.begin()) {
auto it = ctx.begin();
auto end = ctx.end();
detail::maybe_set_debug_format(underlying_, true);
//detail::maybe_set_debug_format(underlying_, true);
Copy link
Contributor Author

@matt77hias matt77hias May 26, 2024

Choose a reason for hiding this comment

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

I end up with something like: ''''A''''''''s''''''''s''''''''e''''''''r''''''''t''\ etc. because of that unconditional debug format setting. If I comment that line, it works for me locally. I use {} as format specifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to set it conditionally for non-string kinds.

@matt77hias matt77hias force-pushed the range_format_string branch 2 times, most recently from 9a7743a to 4fe9f85 Compare May 27, 2024 18:48
@matt77hias matt77hias closed this May 27, 2024
@matt77hias matt77hias reopened this May 27, 2024
@matt77hias matt77hias force-pushed the range_format_string branch 3 times, most recently from 5d127bd to 5e5bd30 Compare May 27, 2024 19:33
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.

Thinking more of it, looks like we don't need to use range_formatter at all for strings and debug strings: https://eel.is/c++draft/format#range.fmtstr.

@matt77hias
Copy link
Contributor Author

matt77hias commented May 28, 2024

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
Copy link
Contributor

vitaut commented May 28, 2024

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
Copy link
Contributor Author

matt77hias commented May 28, 2024

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
Copy link
Contributor Author

matt77hias commented May 29, 2024

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.

@matt77hias matt77hias force-pushed the range_format_string branch 2 times, most recently from a4d947c to e8d9b17 Compare May 29, 2024 18:10
@@ -607,6 +612,47 @@ struct formatter<
}
};

#ifdef __cpp_lib_ranges
Copy link
Contributor

Choose a reason for hiding this comment

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

Making this formatter conditional on ranges can be problematic. {fmt} already has replacement for std::ranges::begin/end (detail::range_begin/end) so let's use them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fmt::basic_string_view would first need a constructor that accepts an iterator/sentinel pair (or iterator/iterator pair).

Copy link
Contributor

Choose a reason for hiding this comment

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

string_view requires a pointer so iterator/sentinel should be converted to pointer/size which better be done outside of the ctor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more of it, I think it's OK to keep this conditional but let's at least kill the dependency on<ranges> and check __cpp_lib_concepts for contiguous iterator support instead.

Copy link
Contributor Author

@matt77hias matt77hias Jun 13, 2024

Choose a reason for hiding this comment

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

Reduced the scope of the ranges dependency.

@vitaut
Copy link
Contributor

vitaut commented Jun 13, 2024

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

# include <tuple>
# include <type_traits>
# include <utility>
# ifdef __cpp_lib_ranges
Copy link
Contributor

@phprus phprus Jun 13, 2024

Choose a reason for hiding this comment

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

@vitaut
It might be better to check like this:

fmt/include/fmt/std.h

Lines 46 to 48 in c4ea903

# if FMT_HAS_INCLUDE(<expected>) && FMT_CPLUSPLUS > 202002L
# include <expected>
# endif

 #  if FMT_HAS_INCLUDE(<ranges>) && FMT_CPLUSPLUS > 201703L 
 #    include <ranges> 
 #  endif

?

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid implicit dependencies.

__cpp_lib_ranges is defined in headers:

  • <algorithm>
  • <functional>
  • <iterator>
  • <memory>
  • <ranges>

https://en.cppreference.com/w/cpp/feature_test#cpp_lib_ranges

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop the dependency here but it can be done separately.

@vitaut vitaut merged commit 794df69 into fmtlib:master Jun 13, 2024
43 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jun 13, 2024

Merged, thanks!

@matt77hias
Copy link
Contributor Author

matt77hias commented Jun 14, 2024

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.

@matt77hias matt77hias deleted the range_format_string branch June 17, 2024 19:48
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