[libcxx-commits] [PATCH] D148826: [libc++][format] Removes vector dependency.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/__format/buffer.h:553
+  _LIBCPP_HIDE_FROM_ABI explicit __retarget_buffer(size_t __size_hint) {
+#  if _LIBCPP_STD_VER >= 23
+    allocation_result __result = std::allocate_at_least(__alloc_, __size_hint ? __size_hint : 256 / sizeof(_CharT));
----------------
philnik wrote:
> Why the `#ifdef`? `__allocate_at_least` was specifically designed to be able to write `auto __alloc_result = std::__allocate_at_least(...)`. (Although, thinking about it we should just have a converting constructor from `allocation_result`)
Changed to this version.


================
Comment at: libcxx/include/__format/buffer.h:596-597
+
+    std::uninitialized_default_construct_n(__ptr_ + __size_, __n);
+    std::transform(__first, __last, __ptr_ + __size_, std::move(__operation));
+    __size_ += __n;
----------------
ldionne wrote:
> Could you confirm that for `char`, `std::uninitialized_default_construct_n(__ptr_ + __size_, __n);` is optimized out?
I verified it -O1 suffices https://godbolt.org/z/Ex5cafPfY


================
Comment at: libcxx/include/__format/buffer.h:622
+#  endif
+    std::uninitialized_move_n(__ptr_, __size_, __result.ptr);
+    ranges::destroy_n(__ptr_, __size_);
----------------
ldionne wrote:
> If this line somehow throws, we'll leak the allocation result above. This is definitely a nitpick cause it should never throw, I guess (those are characters).
> 
> I won't feel strongly if you don't think it's worth addressing, however if you do want to address it, I think we could try to modify `__allocation_guard` so that it plays nicely with `__allocate_at_least`.
I do not really like to modify the `__allocation_guard` IMO we have an `___exception_guard` which can do the same without additional effort. (In fact it seems `__allocation_guard` is only used once.

This too is optimized away https://godbolt.org/z/c71GzE63o so I added it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148826