[libcxx-commits] [PATCH] D110497: [libc++][format][3/6] Adds a __container_buffer.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/buffer.h:148
+  array<_CharT, __buffer_size> __buffer_;
+  _CharT* __buffer_it_{__buffer_.begin()};
+  _Container* __container_;
----------------
Quuxplusone wrote:
> Style nit: `_CharT* __buffer_it_ = __buffer_.begin();` is easier to read, and equivalent. Could even say `__buffer_.data()` and then I wouldn't need to consciously avoid looking up on cppreference whether `array::iterator` is guaranteed to be `_CharT*`. ;)
I prefer the `begin()` since that conveys the intended usage better.


================
Comment at: libcxx/include/list:867
 #endif
+    typedef list<_Tp,_Alloc>                         __enable_format_insertable;
 
----------------
Quuxplusone wrote:
> Style nit: `_Tp, _Alloc` with a space
Interesting clang-format didn't catch that for me.


================
Comment at: libcxx/test/libcxx/utilities/format/enable_insertable.compile.pass.cpp:29-51
+template <class CharT>
+struct no_value_type {
+  using __enable_format_insertable = no_value_type<CharT>;
+  using iterator = CharT*;
+  iterator end();
+  void insert(iterator, CharT*, CharT*);
+};
----------------
Quuxplusone wrote:
> FWIW, I think it is //not// useful to test `no_value_type`, `no_end`, or `no_insert`, because we don't permit users to define such types and we don't expect to define any such types ourselves. Seeing a correct "specialization" of `__enable_format_insertable` should be enough to indicate that the type author intends the type to be insertable. If it's somehow //not// insertable, then that's a serious show-stopping bug in the type, not something that `<format>` should be silently working-around.
I not entirely convinced that removing them is a good idea. I'd like to hear @ldionne's opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110497