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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante 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.
----------------
Quuxplusone wrote:
> /that too/it is too/?
I'll leave this as is since the same block is copied several times. Still hope we soon only need to support compilers with proper concepts support.


================
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:
----------------
Quuxplusone wrote:
> 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.
This is what clang-format makes of it. I noticed clang-format-13 seems to have some support for concepts and I still want to reformat all format related headers once we switch to clang-format-13 and the number of active format patches is reduced.


================
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>>);
----------------
Quuxplusone wrote:
> 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/`?
See D110501.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110495