[libc++] Refactor vector move constructor with allocator

[winner245]

This PR simplifies the implementation of std::vector move constructor with alternative allocator by invoking __init_with_size, instead of calling assign which ultimately calls __assign_with_size. The advantage of using __init_with_size lies in its internal use of exception guard, thus simplifying the code. Furthermore, from a semantic standpoint, it is more intuitive for a constructor to call an initialization function rather than an assignment function.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This PR simplifies the implementation of std::vector move constructor with alternative allocator by invoking __init_with_size, instead of calling assign which ultimately calls __assign_with_size. The advantage of using __init_with_size lies in its internal use of exception guard, thus simplifying the code. Furthermore, from a semantic standpoint, it is more intuitive for a constructor to call an initialization function rather than an assignment function.

---

Full diff: https://github.com/llvm/llvm-project/pull/116449.diff

1 Files Affected:

• (modified) libcxx/include/__vector/vector.h (+1-3)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index d2d707d8c913c0..ac07750d8ece2f 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -976,9 +976,7 @@ vector<_Tp, _Allocator>::vector(vector&& __x, const __type_identity_t<allocator_
     __x.__begin_ = __x.__end_ = __x.__end_cap() = nullptr;
   } else {
     typedef move_iterator<iterator> _Ip;
-    auto __guard = std::__make_exception_guard(__destroy_vector(*this));
-    assign(_Ip(__x.begin()), _Ip(__x.end()));
-    __guard.__complete();
+    __init_with_size(_Ip(__x.begin()), _Ip(__x.end()), __x.size());
   }
 }
 

[philnik777]

LGTM assuming the CI is happy.

[ldionne]

The advantage of using __init_with_size lies in its internal use of exception guard, thus simplifying the code.

Does this patch add an exception guard that wasn't there before? If so, are we fixing an exception safety issue that we were not handling before? If that's the case, this needs a test.

[philnik777]

The advantage of using __init_with_size lies in its internal use of exception guard, thus simplifying the code.

Does this patch add an exception guard that wasn't there before? If so, are we fixing an exception safety issue that we were not handling before? If that's the case, this needs a test.

No, we're simply re-using a different one, which allows us to remove one.

[winner245]

Does this patch add an exception guard that wasn't there before? If so, are we fixing an exception safety issue that we were not handling before? If that's the case, this needs a test.

The exception guard was there before, but the guard was applied around an assign call, which is more verbose than directly calling __init_with_size which has an internal exception guard already. So exception tests available in /vector/vector.cons/exceptions.pass.cpp also already covered this constructor.

[winner245]

The existing exception test for this constructor is problematic as it did not throw any exceptions. I have added a new test for this constructor. Additionally, I found other tests in exceptions.pass.cpp that are similarly problematic: either they did not throw exceptions at all, or they did not throw at the intended points. These issues have been separately addressed in #117662.

[ldionne]

LGTM. I pulled the patch locally and can confirm that the test before the patch doesn't actually test anything. So this both refactors the code and also makes sure that the test is working properly.

Since this is a small change, let's land this before the other vector refactorings.