[libcxx-commits] [PATCH] D128146: [libc++] Use uninitialized algorithms for vector

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:358
 
 // Destroys every element in the range [first, last) FROM RIGHT TO LEFT using allocator
 // destruction. If elements are themselves C-style arrays, they are recursively destroyed
----------------
Can you please add a TODO to switch this to a normal left-to-right algorithm and to use `reverse_iterator` from callers?


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:504-505
+template <class _Alloc, class _Iter, class _Sent>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 void
+__allocator_destroy(_Alloc& __alloc, _Iter __first, _Sent __last) {
+  for (; __first != __last; ++__first)
----------------
Please comment on what this function does.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:512
+// that __first2 can hold at least distance(__first1, __last1) uninitialized elements. If an exception is thrown the
+// already copied elements are destroyed in order of their construction.
+template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
----------------
We should destroy in reverse order of construction, it's usually what's expected. Applies everywhere.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:517-518
+  auto __destruct_first = __first2;
+  auto __guard =
+      std::__make_cxx03_transaction(&__allocator_destroy<_Alloc, _Iter2, _Iter2>, __alloc, __destruct_first, __first2);
+  while (__first1 != __last1) {
----------------
Instead of creating a separate `__transaction` class for C++03, I would do this:

```
template <class _Alloc, class _Iter>
struct _AllocatorDestroyRange {
  _LIBCPP_CONSTEXPR_AFTER_CXX11 void operator()() const {
    std::__allocator_destroy(__alloc_, __first, __last);
  }
  _Alloc& __alloc_;
  _Iter& __first;
  _Iter& __last;
};
```

And then I'd use this from the `__uninitialized_FOO` functions as:

```
__transaction<_AllocatorDestroyRange<_Alloc, _Iter1> > __guard(_AllocatorDestroyRange<_Alloc, _Iter1>(__allloc, __destruct_first, __first2));
```

Basically, I don't like that we are creating our own local emulation of `std::bind`/`std::bind_back` just for `__transaction`.

Another option would be something like

```
auto __guard = std::__make_transaction(std::bind_back(&__allocator_destroy<_Alloc, _Iter2, _Iter2>, __alloc, __destruct_first, __first2));
```

However, `bind_back` is not available in C++03 and I'm not 100% sure it would be a good idea to drag in that dependency.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:528-532
+template <class _Alloc, class _Type>
+struct __allocator_has_trivial_construct : _Not<__has_construct<_Alloc, _Type*, const _Type&> > {};
+
+template <class _Type>
+struct __allocator_has_trivial_construct<allocator<_Type>, _Type> : true_type {};
----------------
Perhaps this should be called `__allocator_has_trivial_copy_construct` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146