[libcxx-commits] [PATCH] D148826: [libc++][format] Removes vector dependency.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 27 09:30:27 PDT 2023
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
More information about the libcxx-commits
mailing list