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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with a comment I'd like to see addressed, but I don't think I need to see this again. IIUC, @vitaut 's concern about character-by-character output has been resolved since this was rebased. We are still buffering too much to @vitaut 's liking, but @Mordante 's telling me that he'll investigate that in the future, before we ship this.

Also, this should be rebased onto the `__get_container()` patch.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110497