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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

I think it's up to @ldionne and/or @vitaut to say whether this PR is ultimately a good tradeoff, complexity-and-performance-wise. IMHO we've successfully reached the local optimum, code-wise. :)



================
Comment at: libcxx/include/__format/buffer.h:148
+  array<_CharT, __buffer_size> __buffer_;
+  _CharT* __buffer_it_{__buffer_.begin()};
+  _Container* __container_;
----------------
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*`. ;)


================
Comment at: libcxx/include/list:867
 #endif
+    typedef list<_Tp,_Alloc>                         __enable_format_insertable;
 
----------------
Style nit: `_Tp, _Alloc` with a space


================
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*);
+};
----------------
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.


================
Comment at: libcxx/test/libcxx/utilities/format/enable_insertable.compile.pass.cpp:68
+  void insert(iterator, CharT*, CharT*);
+};
+
----------------
Also test
```
struct UserVector : public std::vector<char> {
    using std::vector<char>::vector;
};
static_assert(!std::__format::__insertable<UserVector>);
```
This is important to test because users //are// allowed to do stupid things like this. (They //shouldn't//; it's bad style and has many downsides; but they are //allowed// to.)


================
Comment at: libcxx/test/libcxx/utilities/format/enable_insertable.compile.pass.cpp:93
+static_assert(std::__format::__insertable<std::vector<char>>);
+static_assert(std::__format::__insertable<std::vector<wchar_t>>);
+static_assert(std::__format::__insertable<std::deque<char>>);
----------------
What do you expect to happen for `std::vector<int>` or `std::vector<std::string>`? On the one hand, `int` and `string` are not "char types," right? But on the other hand, `vec.insert(vec.end(), charptr, charendptr)` will work as expected, so we //do// want to do the optimization, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110497