[libcxx-commits] [PATCH] D66262: Constrain tuple/unique_ptr move constructors (2899)

Wed Sep 4 11:28:53 PDT 2019

mclow.lists added inline comments.

================
Comment at: libcxx/include/memory:2514

+  template<class _Dummy = typename remove_const<typename remove_reference<_Dp>::type>::type>
   _LIBCPP_INLINE_VISIBILITY
----------------
zoecarver wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > zoecarver wrote:
> > > > > The issue said to use `is_move_assignable_v<D>` but, that will cause some deleters to break. Thoughts on a better way to resolve this? Or should we make people change how they pass their deleter types?
> > > > Deleters are allowed to be lvalue references to function objects, so I don't think you want to use `remove_reference` here (nor `remove_const`). I think it is expected that a `unique_ptr<T, Deleter const&>` should be expected _not_ to be move assignable.
> > > Alrighty. That does seem more correct. But, it will force us to change our tests, and when we have to change our tests, people will probably have to change their code. Given how often `unique_ptr` is used, it may not be desirable to break people's code without guarding it to another version (they might expect it when switching from C++17 to C++2a, but it might be bad for them to re-build libcxx and have their program stop working). 
> > > 
> > > That being said, we should implement the standard, so I will make the change. 
> > I think what would be useful is to have @mclow.lists 's opinion on this matter. Marshall, do you know whether it was understood that the LWG issue resolution would break some code? Was it deemed OK because it only breaks `unique_ptr` using something like a `Deleter const&` (which should be rare)? Are we misunderstanding something?
> Agreed. @mclow.lists friendly ping. What are your thoughts on this?
What's the breakage that you're seeing here?

If you're worried about `op=` on a `unique_ptr<T, const D&>` - I don't see how that would have ever worked.

Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66262