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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 2 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/memory:2489
   _LIBCPP_INLINE_VISIBILITY
-  unique_ptr(unique_ptr&& __u) _NOEXCEPT
+  unique_ptr(unique_ptr&& __u, typename enable_if<
+                                 is_move_constructible<_Dummy>::value,
----------------
ldionne wrote:
> Any reason for preferring this style of `enable_if` (as a default argument) as opposed to using a default _template_ argument?
For some reason the first time I tried this it didn't work, so I looked at `shared_ptr` (where I had remembered something similar), and this is how it was implemented. Just tried it again and it did seem to work, so I will use your suggestion. 


================
Comment at: libcxx/include/memory:2514
 
+  template<class _Dummy = typename remove_const<typename remove_reference<_Dp>::type>::type>
   _LIBCPP_INLINE_VISIBILITY
----------------
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. 


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66262