[libcxx-commits] [PATCH] D154017: Cleanup __uninitialized_temporary_buffer internals.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/inplace_merge.h:228
     difference_type __buf_size = _VSTD::min(__len1, __len2);
-    auto __buf = std::__make_uninitialized_buffer<value_type[]>(nothrow, __buf_size);
     return std::__inplace_merge<_AlgPolicy>(
----------------
We were using `nothrow` to make it clear that this will return `nullptr` instead of throwing if the allocation fails. This was initially designed in the context of making sure that exception safety is really easy to determine since this will be used inside PSTL, where we want to have strict control over what can and can't throw (because we need to exclusively propagate exceptions that originate from the library "setup" code, but terminate for any other user-thrown exception).

I still feel like `nothrow` has its place here in that context.


================
Comment at: libcxx/include/__memory/uninitialized_buffer.h:33
+  _LIBCPP_HIDE_FROM_ABI
+  explicit _LIBCPP_CONSTEXPR  __uninitialized_buffer_deleter(size_t __count = 0) _NOEXCEPT
+      : __count_(__count) {}
----------------
I don't think we need the default argument here since you're always passing an argument to the constructor below.


================
Comment at: libcxx/include/__memory/uninitialized_buffer.h:35
   size_t __count_;
-  _Destructor __destructor_;
 
----------------
IIUC, you're not destroying the elements in the buffer anymore, which seems like a bug.


================
Comment at: libcxx/include/__memory/uninitialized_buffer.h:48
+__make_uninitialized_buffer(size_t __count) {
+  _Tp* __ptr = static_cast<_Tp*>(std::__libcpp_allocate(sizeof(_Tp) * __count, _LIBCPP_ALIGNOF(_Tp), nothrow));
 
----------------
`__libcpp_allocate` vs `operator new` is a great catch, thanks for catching that.


================
Comment at: libcxx/include/__memory/uninitialized_buffer.h:66-67
-__make_uninitialized_buffer(nothrow_t, size_t __count, _Destructor __destructor = __noop()) {
-  static_assert(is_array<_Array>::value, "");
-  using _Tp = __remove_extent_t<_Array>;
 
----------------
We were using the `_Tp[]` syntax so that you would get an array specialization of `unique_ptr<_Tp[]>`, which then supports nice things like `operator[]`. I still think it makes sense to do that.


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

https://reviews.llvm.org/D154017