[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
Thu Jul 6 19:10:19 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:
> var-const wrote:
> > 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?
> This is what we do in other places with the same pattern:
>
> ```
> (void*)std::addressof(*__guard.__get()));
> ```
>
> You can see this at play for example in `libcxx/include/__memory/shared_ptr.h` around `shared_ptr<_Tp> allocate_shared(const _Alloc& __a, _Args&& ...__args)`.
Done. I've had to make `__allocation_guard` follow the rule of 5.
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