[libcxx-commits] [PATCH] D62274: shared_ptr deleter requirements (2802)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

LGTM except for the move issue (which is a really good catch BTW, I hadn't thought about that at all).



================
Comment at: libcxx/include/memory:3015
     {
         __d(__p);
         throw;
----------------
zoecarver wrote:
> I was just thinking, is this going to be problematic if `__d` has already been moved?
Good observation. I think it's an issue, but it's really easy to fix. First, do we agree that line `3010` can't throw? Second, do we agree that we can assume move constructors don't throw?

So we only have to see whether something can throw on line `3009` after the deleter has been moved out of.

- Clearly, copying or moving `__p` around isn't an issue, since it's a pointer.
- Moving `__d` isn't an issue because we said move constructors don't throw.
- Copying `__a` is potentially problematic, since it could throw. But if we use `std::move(__a)` instead, it's not an issue anymore.

None of the stuff inside the implementation of `__shared_ptr_pointer::__shared_ptr_pointer` is an issue either, since it just moves stuff into the compressed pairs, so the same logic applies.

This reasoning applies to all 4 constructors being touched by this patch. So I think we only need to use `std::move(__a)` and we're safe even from a very pedantic point of view.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62274