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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked an inline comment as done.
var-const added inline comments.


================
Comment at: libcxx/include/__memory/allocation_guard.h:48
 template<class _Alloc>
 struct __allocation_guard {
     using _Pointer = typename allocator_traits<_Alloc>::pointer;
----------------
ldionne wrote:
> 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. 🤦🏼‍♂️
Added some tests, please let me know what you think.


================
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();
----------------
ldionne wrote:
> 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.
Done. I think it's cleaner that way as well, it's more obvious what exactly the guard is protecting.


================
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();
----------------
ldionne wrote:
> 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.
Switched back to using `__node_traits::construct`.

Re. testing -- it seems like this would mean adding tests to all the functions that might allocate (IMO conceptually there's no reason to add the test only to `insert_after`), which I think is a bit overkill for this patch. In the future, I'd be happy to add a suite of tests like `allocator_aware.pass.cpp` to check all the allocator-related requirements in one place.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152327