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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision now requires review to proceed.


================
Comment at: libcxx/include/memory:3015
     {
         __d(__p);
         throw;
----------------
zoecarver wrote:
> ldionne wrote:
> > 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.
> Hmm. I think move constructors //shouldn't// throw, but are allowed to. It's not undefined behavior to have a throwing move constructor (unless I'm mistaken, which is entirely possible). But I think you're right, the only thing we're really worried about is the allocator throwing. So what if we just put the call to `.allocate` inside of the try/catch? I'll update this patch to do that. 
Yes, they are allowed to throw, but in that case we don't provide the strong exception guarantee, i.e. the code already behaves correctly. From cppreference:

> To make the strong exception guarantee possible, user-defined move constructors should not throw exceptions. For example, `std::vector` relies on `std::move_if_noexcept` to choose between move and copy when the elements need to be relocated.

But anyway, scratch this discussion. I checked the requirements for this constructor, and it requires that constructing the deleter doesn't throw an exception. Furthermore, `Allocators` are always required to not throw exceptions when copied or moved. So there's no issue here, and the code is correct as-is. We can still use `std::move(__a)` for "efficiency", but there's not difference in correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62274