-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Added range_format::string formatter #3973
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
Conversation
da757f6 to
12fca71
Compare
|
Partially specializing template< mage::Character C >
struct range_format_kind< mage::BasicStringView< C >, C >
: mage::ValueConstant< range_format::string >
{}; |
e6557dd to
4684c03
Compare
|
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. |
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.
Thanks for the PR! Please add a test case to ranges-test and address comments.
2939345 to
902e4de
Compare
include/fmt/ranges.h
Outdated
| auto it = ctx.begin(); | ||
| auto end = ctx.end(); | ||
| detail::maybe_set_debug_format(underlying_, true); | ||
| //detail::maybe_set_debug_format(underlying_, true); |
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 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.
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.
You'll need to set it conditionally for non-string kinds.
9a7743a to
4fe9f85
Compare
4fe9f85 to
c8f1b4e
Compare
5d127bd to
5e5bd30
Compare
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.
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.
|
|
|
If you are referring to |
|
Ok in that case I think it would make sense to use something like contiguous_range + sized_range -> basic_string_view (via explicit range constructor) |
3ad0f17 to
706eabd
Compare
|
I use If |
a4d947c to
e8d9b17
Compare
include/fmt/ranges.h
Outdated
| } | ||
| }; | ||
|
|
||
| #ifdef __cpp_lib_ranges |
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.
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.
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.
fmt::basic_string_view would first need a constructor that accepts an iterator/sentinel pair (or iterator/iterator pair).
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.
string_view requires a pointer so iterator/sentinel should be converted to pointer/size which better be done outside of the ctor.
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.
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.
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.
Reduced the scope of the ranges dependency.
|
@matt77hias do you still plan to update this PR to address the comment (remove |
e8d9b17 to
3e3062c
Compare
1473b8f to
1eb4c76
Compare
1eb4c76 to
94f3616
Compare
| # include <tuple> | ||
| # include <type_traits> | ||
| # include <utility> | ||
| # ifdef __cpp_lib_ranges |
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.
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.
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
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 can drop the dependency here but it can be done separately.
|
Merged, thanks! |
|
Ah ok now I understand how the ranges header could have been removed entirely. I didn't go that far because fmt's own |
Either the basic_string_view range constructor or a user-defined conversion operator will be used.