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

[Mark de Wever via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__format/buffer.h:109
+template <class _Container>
+concept __insertable = __enable_insertable<_Container> &&
+    __formatter::__char_type<typename _Container::value_type> &&
----------------
Quuxplusone wrote:
> The machinery to `__enable_insertable` in a container right now is like 16 lines. You could get it down to 1 line, and no helper headers, and C++11-compatible, if you changed this to
> ```
> is_same_v<_Container, typename _Container::__enable_format_insertable>
> ```
> Then the containers (vector, deque, string, list, etc.) would just have to say e.g.
> ```
>     using __enable_format_insertable = vector<_Tp, _Alloc>;
> ```
> without any extra headers or ifdef-guards or anything.
Thanks for the suggestion, this indeed is a lot less intrusive.


================
Comment at: libcxx/include/__format/buffer.h:117-139
+/// Extract the container type of a \ref back_insert_iterator.
+template <class _It>
+struct _LIBCPP_TEMPLATE_VIS __back_insert_iterator_container {
+  using type = void;
+};
+
+template <__insertable _Container>
----------------
Quuxplusone wrote:
> Rather than lines 129-140, I suggest that you add a member function `container_type *back_insert_iterator::__get_container() const` to libc++'s `back_insert_iterator`.
> I suppose you still need a partial-specialization-based approach equivalent to lines 117-127, in order to tell you whether the user's iterator type is //exactly// `back_insert_iterator` (in which case this optimization is safe) or merely something that //slices to// `back_insert_iterator` (in which case this optimization is dubious). But once you know it's exactly `back_insert_iterator`, you can just ask for its `container_type` member typedef, and use your new member function to get a pointer to the container; you don't have to use tricks for //that// part.
See D110573.


================
Comment at: libcxx/include/__format/buffer.h:171
+
+  array<_CharT, __buffer_size> __buffer_{};
+  _CharT* __buffer_it_{__buffer_.begin()};
----------------
Quuxplusone wrote:
> Why not `_CharT __buffer_[__buffer_size];`? (What does the `std::array` dependency buy you? and the zero-initialization?)
I prefer an `array` just for convenience. I need to know the last element and I like it better to use `end()`.
The zero-initialization isn't needed.


================
Comment at: libcxx/include/module.modulemap:442
       module buffer                 { private header "__format/buffer.h"                 }
+      module enable_intertable      { private header "__format/enable_insertable.h"      }
       module format_error           { private header "__format/format_error.h"           }
----------------
Quuxplusone wrote:
> /intertable/insertable/
This is no longer needed due to your earlier suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110497