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

[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/__memory/uninitialized_algorithms.h:364-366
 template<class _Alloc, class _BidirIter, class = __enable_if_t<
-    __is_cpp17_bidirectional_iterator<_BidirIter>::value
->>
+    __is_cpp17_bidirectional_iterator<_BidirIter>::value && is_array<__iter_value_type<_BidirIter> >::value
+> >
----------------
Let's use the same pattern for `__enable_if_t` here, i.e. use a non-type template parameter like you do below.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:389-394
+template <
+    class _Alloc,
+    class _BidirIter,
+    __enable_if_t<
+        __is_cpp17_bidirectional_iterator<_BidirIter>::value && !is_array<__iter_value_type<_BidirIter> >::value,
+        int> = 0>
----------------
We should figure out what clang-format does wrong here, but in the meantime I would rather use the same formatting as line 364-366.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:515
+// that __first2 can hold at least distance(__first1, __last1) uninitialized elements. If an exception is thrown the
+// already copied elements are destroyed again.
+template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
----------------
Please also add a note that array elements are NOT treated specially by this function. We may also want to differentiate between functions in this file that handle arrays vs those that don't, since it's really not obvious from their current names.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:524
+    while (__first1 != __last1) {
+      allocator_traits<_Alloc>::construct(__alloc, std::__to_address(__first2), *__first1);
+      ++__first1;
----------------
I think you either need to construct array elements recursively *and* destroy them recursively, or not. But construction and destruction has to be consistent w.r.t. how it handles array elements. Otherwise, you'll get a mismatching number of calls to `allocator_traits::construct` and `allocator_traits::destroy`.

Concretely, I think for `std::vector` you don't want to treat array elements specially. So I would add a `std::__allocator_destroy(_Alloc&, _Iter, _Sent)` function and call that in the `catch (...)` instead.

This should be tested by ensuring that we have a matching number of calls to construct and destroy when we use this algorithm with array elements.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:549
+  auto __diff = __last1 - __first1;
+  ::__builtin_memcpy(const_cast<_RawType1*>(__first2), __first1, __diff * sizeof(_Type1));
+  return __first2 + __diff;
----------------
Here, I suggest this instead:

```
template <class _Alloc,
          class _Type,
          class _RawType = typename remove_const<_Type>::type,
          __enable_if_t<
            // using _RawType because of the allocator<T const> extension
            is_trivially_copy_constructible<_RawType>::value && 
            is_trivially_copy_assignable<_RawType>::value &&
            (__is_default_allocator<_Alloc>::value || !__has_construct<_Alloc, _RawType*, _Type const&>::value)
          >* = nullptr>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 _Type*
__uninitialized_allocator_copy(_Alloc&, _Type const* __first1, _Type const* __last1, _Type* __first2) {
  // TODO: Remove the const_cast once we drop support for std::allocator<T const>
  return std::copy(__first1, __last1, const_cast<_RawType*>(__first2));
}
```

In particular, note the tweaked `enable_if` conditions -- I think this is what we need to be safe here. If the type is not trivially copy constructible, we MUST call its copy constructor (instead of the assignment in `std::copy`), otherwise the optimization is not transparent.

If the type is not trivially assignable, then we can also notice that we are doing an assignment instead of a copy-construction here, and so it's not transparent.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:555
+// move constructor is noexcept. Otherwise try to copy all elements. If an exception is thrown the already copied
+// elements are destroyed again.
+template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
----------------



================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:576
+  } catch (...) {
+    std::__allocator_destroy_multidimensional(__alloc, __destruct_first, __first2);
+    throw;
----------------
Same comment, we shouldn't treat array types specially here.

Treating array types specially was only meaningful for the `std::make_shared<T[N]>(...)` functions because they were specified that way. Otherwise, I would have never bothered to handle array types specially :-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146