[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:168-175
+template <class _Container>
+concept __insertable =
+    same_as<_Container, typename _Container::__enable_format_insertable> &&
+    __formatter::__char_type<typename _Container::value_type> &&
+    requires(_Container& __t, typename _Container::value_type* __first,
+             typename _Container::value_type* __last) {
+  __t.insert(__t.end(), __first, __last);
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > Is there any reason why this isn't implemented as a `bool` variable template that containers specialize or something similar? IMO it would be nice to keep containers free of a `__enable_format_insertable` typedef used for this sole purpose.
> > > 
> > > Requiring a specialization also makes it slightly less likely that users are going to do it for their own containers, which IMO is a good thing (we really don't want users to start depending on this unless the Standard commits to providing this functionality).
> > > 
> > > [Reading some other comments, I get the feeling this might have been how it was implemented in previous diffs, please let me know if that's the case]
> > That was my initial approach https://reviews.llvm.org/D110497?vs=on&id=375096#toc
> > when you too like that better I can switch back to that approach.
> Yeah, I think it would be better to have a less intrusive mechanism. We can specialize it for various containers in this header instead of having to touch `vector` & friends directly.
In that case format pulls in several unrelated headers. In the original approach these container headers pull in one small header. So I think that's the better solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110497