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

[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/allocation_guard.h:48
 template<class _Alloc>
 struct __allocation_guard {
     using _Pointer = typename allocator_traits<_Alloc>::pointer;
----------------
I feel bad for not doing this when I wrote the class (IIRC), but we should add a few basic `libcxx/test/libcxx` tests for basic functionality. Don't go crazy, but at least basic usage. If I had done that, I wouldn't have written a class that has 4x incorrectly defined special members. 🤦🏼‍♂️


================
Comment at: libcxx/include/forward_list:1264-1266
+        _Guard __h(__a, 1);
+        std::__construct_at(std::addressof(*__h.__get()), __v, /*__next_ =*/nullptr);
+        __node_pointer __first = __h.__release_ptr();
----------------
Here, I might put these three lines into a scope of their own. That way, we end the lifetime of the guard so we can easily understand it's not needed anymore. Then, within the for loop below, you can just create a new guard variable every time.


================
Comment at: libcxx/include/forward_list:1265
+        _Guard __h(__a, 1);
+        std::__construct_at(std::addressof(*__h.__get()), __v, /*__next_ =*/nullptr);
+        __node_pointer __first = __h.__release_ptr();
----------------
You're bypassing any allocator's `::construct` method in the new patch. I think you want to keep using `_node_traits::construct` instead.

It should be easy to add a test with a simple custom allocator where you can check that its `construct` method has been called.


================
Comment at: libcxx/include/forward_list:1275
+                __h = _Guard(__a, 1);
+                std::__construct_at(std::addressof(*__h.__get()), __v, /*__next_ =*/nullptr);
+                __last->__next_ = __h.__release_ptr();
----------------
Same about `::construct`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152327