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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/memory:3015
     {
         __d(__p);
         throw;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62274