[libcxx-commits] [PATCH] D129964: [libc++][format] Improve format buffer.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done and 3 inline comments as not done.
Mordante added a comment.

Thanks for the review!

In D129964#3683530 <https://reviews.llvm.org/D129964#3683530>, @vitaut wrote:

> Nice improvement but I'd still recommend exploring direct writes to contiguous containers as discussed on the other diff.

I could explore that, but I want to see what the overhead is. I already wrap a contiguous storage as a direct buffer, which has a capacity of `size_t(-1)`. So these write directly end up in the underlaying storage. The only overhead should be one size check for every write operation. This change shows a large improvement there too since instead of checking the capacity every character it is checked once per write operation.

Next I want to look at the `std::back_inserter<std::string>`. That operation still stores all data in a temporary buffer before copying the entire buffer to the string. There I expect it will be possible to improve the performance of `std::format` itself.



================
Comment at: libcxx/include/__format/buffer.h:89
+  template <__formatter::__char_type _InCharT>
+  _LIBCPP_HIDE_FROM_ABI void __copy(basic_string_view<_InCharT> __str) {
+    // When the underlying iterator is a simple iterator the __capacity_ is
----------------
vitaut wrote:
> Why are some member functions like `__copy` mangled while others like `make_output_iterator` are not?
The all should be, but I will do the unrelated changes in a separate NFC patch. I noticed it while working on this, the the comment at line 71.


================
Comment at: libcxx/include/__format/formatter_output.h:96
+_LIBCPP_HIDE_FROM_ABI auto
+__copy(basic_string_view<_CharT> __str, output_iterator<const _OutCharT&> auto __out_it) -> decltype(__out_it) {
+  if constexpr (_VSTD::same_as<decltype(__out_it), _VSTD::back_insert_iterator<__format::__output_buffer<_OutCharT>>>) {
----------------
vitaut wrote:
> Why const?
You mean in `output_iterator<const _OutCharT&>`? 
I used that due to http://eel.is/c++draft/format#functions-13
`Constraints: Out satisfies output_­iterator<const charT&>.` Should that wording be different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129964