[libcxx-commits] [PATCH] D138795: [libc++] Don't assume that string_view::const_iterator is a raw pointer

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

Nice catch, I thought the iterators were required to be character pointers, but they indeed are not. So I feel this it the better way to do it. (Unless we want to "unwrap our iterators" in the top-level format functions.)

In D138795#3953174 <https://reviews.llvm.org/D138795#3953174>, @ldionne wrote:

> @Mordante Please let me know if you like this approach. In a few places, we have the choice of blindly taking `_Iterator` or explicitly stating `basic_string_view::const_iterator` -- let me know what you prefer and I'll change it.

In general I like the approach, but I left some comments. I really think we should constrain the iterator types since the code makes assumptions on which operations are allowed.

Otherwise LGTM!



================
Comment at: libcxx/include/__format/format_string.h:32
 struct _LIBCPP_TEMPLATE_VIS __parse_number_result {
-  const _CharT* __ptr;
+  _Iterator __ptr;
   uint32_t __value;
----------------
The current name is misleading after the change.

The original name was based on `<charconv>` which uses `char` pointers. When switching to iterators I prefer to use a more generic name.


================
Comment at: libcxx/include/__format/format_string.h:103
    */
-  const _CharT* __end = __end_input - __begin > 9 ? __begin + 9 : __end_input;
+  _Iterator __end = __end_input - __begin > 9 ? __begin + 9 : __end_input;
   uint32_t __value = *__begin - _CharT('0');
----------------
I think it would be good to add consistent constrains to `_Iterator`. This line requires a random access iterator.
In some places you used `contiguous_iterator`, so I think it would be good to do that consistently.
(All code is C++20 so nothing we can use concepts.)


================
Comment at: libcxx/include/__format/formatter_output.h:255-256
 
-template <class _CharT, class _ParserCharT>
+template <contiguous_iterator _Iterator, class _ParserCharT,
+  class _CharT = iter_value_t<_Iterator>>
 _LIBCPP_HIDE_FROM_ABI auto
----------------
In combination with the next suggestion.


================
Comment at: libcxx/include/__format/formatter_output.h:260
+        _Iterator __last,
         output_iterator<const _CharT&> auto __out_it,
         __format_spec::__parsed_specifications<_ParserCharT> __specs,
----------------
Wouldn't this work?


================
Comment at: libcxx/include/__format/formatter_output.h:271
+template <contiguous_iterator _Iterator, class _ParserCharT,
+  class _CharT = iter_value_t<_Iterator>>
 _LIBCPP_HIDE_FROM_ABI auto
----------------
Likewise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138795/new/

https://reviews.llvm.org/D138795