[libcxx-commits] [PATCH] D112361: [libc++][format] Buffer changes proof-of-concept

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/include/__format/buffer.h:60-61
+
+  template <class _Storage>
+  _LIBCPP_HIDE_FROM_ABI void reset(_Storage& __storage) {
+    __ptr_ = __storage.begin();
----------------
Looks like this doesn't need to be a template because it is only called with internal storage. 


================
Comment at: libcxx/include/__format/buffer.h:70
+
+  _LIBCPP_HIDE_FROM_ABI void push_back(_CharT __c) {
+    __ptr_[__size_++] = __c;
----------------
Mordante wrote:
> vitaut wrote:
> > You might also want to provide an API for appending multiple chars at a time (e.g. a `string_view`) which can be implemented more efficiently than appending individual characters.
> I already considered that, but I prefer to do that in a separate commit.
> I'll add a TODO FMT in the source to make that intention clear.
Makes sense.


================
Comment at: libcxx/include/__format/buffer.h:85-89
+  void (*__flush_)(_CharT*, size_t, void*);
   void* __obj_;
+  _CharT* __ptr_;
+  size_t __capacity_;
+  size_t __size_{0};
----------------
Not sure if it matters but `__ptr_`, `__size_` and `__capacity_` are accessed way more frequently than `__flush_` and `__obj_` so maybe put them first?


================
Comment at: libcxx/include/__format/buffer.h:134-140
+  _CharT* __ptr_;
+  /// The capacity of the storage space.
+  ///
+  /// Normally the code expects the direct storage to have an infinite size.
+  /// The limit is used in format_to_n to limit the number of writes to the
+  /// underlying output.
+  size_t __capacity_;
----------------
`__ptr_` and `__capacity_` duplicate members of `__format_buffer`. Is it possible to get rid of them?


================
Comment at: libcxx/include/__format/buffer.h:230
 template <class _OutIt, class _CharT>
 class _LIBCPP_TEMPLATE_VIS __buffer_selector {
+
----------------
There is a naming inconsistency here (buffer vs writer): the class is called `__buffer_selector` while it selects between `__writer_*`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112361