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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

I'll fix the C++03 failures.



================
Comment at: libcxx/include/memory:3015
     {
         __d(__p);
         throw;
----------------
ldionne wrote:
> 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.
OK. Thanks for checking :) I think using std::move on __a is a separate patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62274