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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/buffer.h:57-59
+        __flush_([](_CharT* __ptr, size_t __size, void* __o) {
+          static_cast<_Tp*>(__o)->flush(__ptr, __size);
+        }),
----------------
vitaut wrote:
> Mordante wrote:
> > vitaut wrote:
> > > As discussed in D112361, it would be good to avoid extra buffering for the common case of appending to a contiguous container such as string or vector.
> > What do you mean with "appending"?
> > `std::back_inserter(std::string)` or `std::string::begin()`
> > 
> > I think it makes sense to buffer the `back_inserter`. For example every `push_back` in a `std::string` NUL-terminates the string. The `insert` operation does one NUL-termination.
> > 
> > D110497 changes the behaviour for these containers to buffer in this object and then use `insert` in these containers.
> > 
> > But I can test with the benchmarks whether the extra buffering has a positive or negative effect.
> > What do you mean with "appending"?
> 
> `std::back_inserter(std::string)`
> 
> > For example every push_back in a std::string NUL-terminates the string.
> 
> You should avoid character-by-character insertion in the first place. Also adding buffering for `std::string` is easy but you cannot remove it so the current approach is inherently less efficient than needed.
I agree and I should improve this inefficiency. I added a todo on line 71 to that effect and have more todos for this in my personal notes.
For now my main focus is feature completeness, so I prefer to postpone these changes to a later time.
D110497 already has some improvements for containers, but for string and vector we can do better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110495