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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Jun 23 11:17:38 PDT 2022

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
+__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
+  try {
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
+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
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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list