[libcxx-commits] [PATCH] D152327: [libc++] Fix an exception safety issue in `forward_list` and add tests.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks a lot for fixing this and adding test coverage! I suspected it was indeed a "silly" mistake in the current code. I have a few comments below.



================
Comment at: libcxx/include/forward_list:1254-1256
         unique_ptr<__node, _Dp> __h(__node_traits::allocate(__a, 1), _Dp(__a, 1));
+        __h->__next_ = nullptr;
         __node_traits::construct(__a, _VSTD::addressof(__h->__value_), __v);
----------------
This makes me wonder whether we should instead be constructing this with e.g. `make_unique`. Why don't we have a proper constructor for `__node` and call that instead, it would make sure that we never forget to initialize some of its members. Do you have thoughts about this?


================
Comment at: libcxx/test/std/containers/exception_safety_helpers.h:13
+#include <cassert>
+#include <utility>
+
----------------
`<cstddef>` is missing, there is a reference to `std::size_t` below.


================
Comment at: libcxx/test/std/containers/exception_safety_helpers.h:15
+
+#if !defined(TEST_HAS_NO_EXCEPTIONS)
+template <int N>
----------------
You need to include `test_macros.h`.


================
Comment at: libcxx/test/std/containers/sequences/forwardlist/robust_against_exceptions.pass.cpp:70
+    // forward_list(size_type n, const value_type& v);
+    test_exception_safety_throwing_copy_iterator_pair([](T* from, T*){
+      std::forward_list<T> c(5, *from);
----------------
The fact that `ThrowingCopy` will throw after 3 copies is hardcoded inside `test_exception_safety_throwing_copy_iterator_pair`, making this a false abstraction IMO. Like I can't understand this test without immediately reading what `test_exception_safety_throwing_copy_iterator_pair` does and understanding that I need to construct at least `3` elements below to make this test work as expected. This makes me wonder whether this is a good candidate for a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152327