[libcxx-commits] [PATCH] D145853: [libc++][format] range-default-formatter for strings.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante created this revision.
Herald added a project: All.
Mordante updated this revision to Diff 504387.
Mordante added a comment.
Mordante updated this revision to Diff 504430.
Mordante marked an inline comment as done.
Mordante published this revision for review.
Mordante added reviewers: ldionne, vitaut.
Mordante marked an inline comment as done.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

CI fixes.


Mordante added a comment.

Addresses review comment.


Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__format/range_default_formatter.h:193-194
+  format(conditional_t<ranges::input_range<const _Rp>, const _Rp&, _Rp&> __range, _FormatContext& __ctx) const {
+    if constexpr (ranges::contiguous_range<_Rp>)
+      return __underlying_.format(basic_string_view<_CharT>{__range.begin(), __range.end()}, __ctx);
+    else {
----------------
I think that the condition should be `ranges::contiguous_range<_Rp> && ranges::sized_sentinel_for<ranges::sentinel_t<_Rp>, ranges::iterator_t<_Rp>>`, since those are the constraints of `basic_string_view`'s constructor (http://eel.is/c++draft/string.view.template#string.view.cons-7). Also `begin()` and `end()` functions are not required to be the members of range:
  lang=cpp
  // https://godbolt.org/z/xzvG5Y36Y
  struct X { };
  int* begin(X);
  int* end(X);
  static_assert(ranges::contiguous_range<X>); // OK!

I think that You should try simpler approach that is already used in range_formatter: https://github.com/llvm/llvm-project/blob/43ae4b62b2671cf73e691c0b53324cd39405cd51/libcxx/include/__format/range_formatter.h#L170-L179


================
Comment at: libcxx/include/__format/range_default_formatter.h:193-194
+  format(conditional_t<ranges::input_range<const _Rp>, const _Rp&, _Rp&> __range, _FormatContext& __ctx) const {
+    if constexpr (ranges::contiguous_range<_Rp>)
+      return __underlying_.format(basic_string_view<_CharT>{__range.begin(), __range.end()}, __ctx);
+    else {
----------------
JMazurkiewicz wrote:
> I think that the condition should be `ranges::contiguous_range<_Rp> && ranges::sized_sentinel_for<ranges::sentinel_t<_Rp>, ranges::iterator_t<_Rp>>`, since those are the constraints of `basic_string_view`'s constructor (http://eel.is/c++draft/string.view.template#string.view.cons-7). Also `begin()` and `end()` functions are not required to be the members of range:
>   lang=cpp
>   // https://godbolt.org/z/xzvG5Y36Y
>   struct X { };
>   int* begin(X);
>   int* end(X);
>   static_assert(ranges::contiguous_range<X>); // OK!
> 
> I think that You should try simpler approach that is already used in range_formatter: https://github.com/llvm/llvm-project/blob/43ae4b62b2671cf73e691c0b53324cd39405cd51/libcxx/include/__format/range_formatter.h#L170-L179
Thanks, good catch. This patch was written quite a while ago and I forgot to update it with the changes in the range_foramatter. (I didn't upload it before since there were issues when using these formatters in ranges, which is addressed by LWG3892.)


Implements the range-default-formatter specialization range_format::string
and range_format::debug_string.

Implements parts of

- P2286R8 Formatting Ranges
- P2585R0 Improving default container formatting

Depends on D145847 <https://reviews.llvm.org/D145847>


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145853

Files:
  libcxx/docs/Status/FormatPaper.csv
  libcxx/include/__format/range_default_formatter.h
  libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.functions.format.pass.cpp
  libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.functions.tests.h
  libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.functions.vformat.pass.cpp
  libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.pass.cpp
  libcxx/test/std/utilities/format/format.range/format.range.fmtstr/parse.pass.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D145853.504430.patch
Type: text/x-patch
Size: 35162 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230312/1348b7db/attachment-0001.bin>