[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:503
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 _Iter2
+__uninitialized_allocator_copy(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1, _Iter2 __first2) {
+  auto __destruct_first = __first2;
----------------
Can you please add a comment explaining what `__uninitialized_allocator_copy` & friends do similar to what we do in `__uninitialized_allocator_fill_n` (and others)?

In particular, I think that explaining the exception safety guarantee offered by each algorithm added here is important.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:506
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  try {
+#endif
----------------
Should we have a `static_assert(__is_cpp17_copy_insertable<...>)` here?


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:525
+          class _Type2,
+          class _RawType1 = typename remove_const<_Type1>::type,
+          class           = __enable_if_t<is_trivially_copy_constructible<_Type1>::value &&
----------------
Let's introduce `RawType2` for symmetry?


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:568
+  auto __diff = __last1 - __first1;
+  ::__builtin_memcpy(__first2, __first1, __diff * sizeof(_Type));
+  return __first2 + __diff;
----------------
If you used `std::copy` (or `std::copy_n`), I think you could simplify this and you wouldn't have to handle `reverse_iterator` specially below.


================
Comment at: libcxx/include/__utility/move.h:30
+#ifdef _LIBCPP_NO_EXCEPTIONS
+template <class _Tp>
+using __move_if_noexcept_result_t = _Tp&&;
----------------
We shouldn't do this. It makes us non-conforming under `-fno-exceptions`. We shouldn't try to do as-if `noexcept(<anything>) == true` *in the library* when `-fno-exceptions` is used. If we wanted to have that behavior, it should be achieved through the compiler.

Also relevant as background: D62228


================
Comment at: libcxx/include/vector:905
+    __v.__begin_ =
+        std::__uninitialized_allocator_move_if_noexcept(__alloc(),
+                                                        _RevIter(__end_),
----------------
The code didn't destroy the new elements before in case of a failure, but now it does. I wonder whether the original code was written that way on purpose?


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan_throw.pass.cpp:106
   assert(is_contiguous_container_asan_correct(v));
 }
 
----------------
We should also have a test that ensures that we destroy the newly created elements if an exception is thrown. I think it was wrong to skip that in the code previously, since that means that we'd have been potentially leaking stuff if an exception was thrown.


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/asan_throw.pass.cpp:102
   } catch (int e) {
-    assert(v.size() == 3);
+    assert(v.size() == 2);
   }
----------------
philnik wrote:
> Mordante wrote:
> > Is this a behaviour change or a bug fix? I don't understand this change. Maybe add a description to the patch that explains this behaviour change. That way when we look at the history we know why this was done.
> https://eel.is/c++draft/vector#modifiers-2 is the relevant paragraph I think. I'm not entirely sure if this is just a behaviour change or if it's a bugfix. I //think// it's a bugfix.
My reading is that we are fixing a bug, since this line of the spec should apply:

> If an exception is thrown other than by the copy constructor, move constructor, assignment operator, or move assignment operator of `T` or by any `InputIterator` operation there are no effects.

Indeed, the exception is thrown by `X(char)`, which is none-of-the-above, and so there should be no effects. Previously, the size of the vector would have been modified, and that's wrong.

This mandates a test in `libcxx/test/std` -- it's a pretty serious bug since exception guarantees in `push_back` & friends are a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128146