[libcxx-commits] [PATCH] D110495: [libc++][format][2/6] Adds a __output_iterator.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__format/buffer.h:29
+// If the compiler has no concepts support, the format header will be disabled.
+// Without concepts support enable_if needs to be used and that too much effort
+// to support compilers with partial C++20 support.
----------------
/that too/it is too/?


================
Comment at: libcxx/include/__format/buffer.h:38
+/// In order to specialize the iterator only for the character type used it
+/// uses a type erased function pointer to call the underlying buffer storage
+/// function.
----------------
Nit: `type-erased` hyphenated. 


================
Comment at: libcxx/include/__format/buffer.h:45-46
+  /// \param __c the character to write.
+  /// \param __obj a valid pointer to an object. This object is a class
+  ///              containing a member function @c put(_CharT).
+  using _Fun = void (*)(_CharT __c, void* __obj);
----------------
Generally I find the javadoc-style comments unhelpful; but also in this case it's a bit misleading, since of course `__obj` can't be //any// old class object containing a `put`; it must be the //specific// object, of the //specific// type, that you instantiated `__put` with (or that you instantiated `__output_iterator`'s ctor with, if you take my refactor below). Fortunately, if you take my refactor below, then you won't need `_Fun` in multiple places and so you won't need a distinct name for it and so you won't need a comment about what the name means. :)


================
Comment at: libcxx/include/__format/buffer.h:72-76
+/// Helper function call the buffer member function from @ref __output_iterator.
+template <class T, __formatter::__char_type _CharT>
+_LIBCPP_HIDE_FROM_ABI void __put(_CharT __c, void* __obj) {
+  static_cast<T*>(__obj)->put(__c);
+}
----------------
The comment is ungrammatical and unhelpful, and `T` needs uglification; but why just not make this a private detail of `__output_iterator`? Instead of
```
  _LIBCPP_HIDE_FROM_ABI explicit __output_iterator(_Fun __fun, void* __obj)
      : __fun_(__fun), __obj_(__obj) {}
```
do
```
  _LIBCPP_HIDE_FROM_ABI explicit __output_iterator(_Tp *__t)
      : __fun_([](_CharT __c, void *__o){ static_cast<_Tp*>(__o)->put(__c); }), __obj_(__t) {}
```
and construct it on line 504 as
```
_VSTD::__format::__output_iterator<_CharT>(_VSTD::addressof(__buffer))
```
(notice `addressof`, not `operator&`, for ADL-proofing and hijacking-avoidance)


================
Comment at: libcxx/include/__format/buffer.h:84-85
+template <class _OutIt, __formatter::__char_type _CharT>
+requires(output_iterator<_OutIt, const _CharT&>) class _LIBCPP_TEMPLATE_VIS
+    __iterator_wrapper_buffer {
+public:
----------------
Btw, I see some inconsistency throughout on whether we're doing 2-space tabs in this file or 4-space (lines 57, 88). Doesn't matter, though.


================
Comment at: libcxx/test/libcxx/utilities/format/format.formatter/format.context/types.compile.pass.cpp:111
+                   std::basic_format_context<
+                       std::__format::__output_iterator<wchar_t>, wchar_t>>);
----------------
Your commit message implies that you did some performance testing benchmarks between this implementation and some other implementation. What were those benchmarks, and can you check them into `libcxx/benchmarks/`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110495