[libcxx-commits] [PATCH] D152327: [libc++] Fix an exception safety issue in `forward_list` and add tests.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Jul 5 21:34:20 PDT 2023
var-const added inline comments.
================
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);
----------------
ldionne wrote:
> ldionne wrote:
> > 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?
> This is what I had in mind:
>
> ```
> unique_ptr<__node, _Dp> __h = std::make_unique<__node>(__node(__v, __next=nullptr) _Dp(__a, 1));
> ```
>
> Now we need to allocate using the right allocator, so we might need to use something like `__allocation_guard` instead:
>
> ```
> __allocation_guard<__node_allocator> __h(__a, 1);
> std::construct_at(__h.__get(), __v, /*next*/nullptr); // this is nicer here cause we call the node's constructor for real
> __node_pointer __first = __h.__release();
> ```
Unfortunately, `construct_at` doesn't work because it expects the argument to be a raw pointer whereas the allocator might define the pointer type to be a class. We could avoid using `construct_at` but I'm not sure this is worth pursuing -- what do you think?
================
Comment at: libcxx/test/std/containers/exception_safety_helpers.h:36
+
+ // Defined to silence GCC warnings. For test purposes, only copy construction is considered `created_by_copying`.
+ ThrowingCopy& operator=(const ThrowingCopy& other) {
----------------
Mordante wrote:
> Would deleting the function have the same effect?
No, `operator=` is still used within instantiated code (even if it's not invoked at runtime), and the compiler complains about overload resolution choosing a deleted function.
================
Comment at: libcxx/test/std/containers/exception_safety_helpers.h:75-76
+ assert(T::created_by_copying == 1);
+ assert(T::destroyed == 0);
+ }
+}
----------------
Mordante wrote:
> Alternatively there is the `TEST_VALIDATE_EXCEPTION` macro which I use in the formatting tests
> `libcxx//data/src/llvm/libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp` for example.
Hmm, is having an `assert(false)` preferable to just letting the wrong exception escape and crash the test?
================
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);
----------------
ldionne wrote:
> 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.
Reworked the function to take the total number of elements created and the number of the throwing copy as template parameters. I also reduced the number of these functions to just two, removing some of the convenience ones (the `_container` one is a bit more difficult to remove because creating the container inside the user callback means the container would get destroyed when the exception is thrown, increasing the number of destroyed elements; then it becomes harder to check how many elements were destroyed inside the helper function).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152327/new/
https://reviews.llvm.org/D152327
More information about the libcxx-commits
mailing list