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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D148826#4290117 <https://reviews.llvm.org/D148826#4290117>, @philnik wrote:

> Do you plan to move this back to `vector`, or is this a permanent change? If this is permanent, could you maybe elaborate a bit on the exact cycle this creates, and why this can't be resolved by moving the code around a bit?

Mark tried to split `<vector>` into `vector<T>` and `vector<bool>` in another revision I don't have handy right now, and we decided not to go down that route.



================
Comment at: libcxx/include/__format/buffer.h:570
+  _LIBCPP_HIDE_FROM_ABI void push_back(_CharT __c) {
+    std::construct_at(__ptr_ + __size_++, __c);
+
----------------
A bit subtle otherwise.


================
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;
----------------
Could you confirm that for `char`, `std::uninitialized_default_construct_n(__ptr_ + __size_, __n);` is optimized out?


================
Comment at: libcxx/include/__format/buffer.h:617-621
+#  if _LIBCPP_STD_VER >= 23
+    allocation_result __result = std::allocate_at_least(__alloc_, __capacity);
+#  else
+    __allocation_result __result = std::__allocate_at_least(__alloc_, __capacity);
+#  endif
----------------
Same comment, personally I think `auto __result = std::__allocate_at_least(__alloc_, __capacity);` would be better, since this is not ABI-sensitive.

If we don't feel comfortable using this trick, then we should change how we designed `__allocate_at_least`.


================
Comment at: libcxx/include/__format/buffer.h:622
+#  endif
+    std::uninitialized_move_n(__ptr_, __size_, __result.ptr);
+    ranges::destroy_n(__ptr_, __size_);
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148826