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

Thu Sep 5 09:02:28 PDT 2019

zoecarver marked an inline comment as done.
zoecarver added inline comments.

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

+  template<class _Dummy = typename remove_const<typename remove_reference<_Dp>::type>::type>
   _LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> ldionne wrote:
> > zoecarver wrote:
> > > mclow.lists wrote:
> > > > 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.
> > > > 
> > > That was what I was worried about. Not sure why, though. I think I may have misdiagnosed the cause of an error while creating this patch. I will update this template to be `class _Dummy = _Dp`.
> > So, my initial point is that our test suite currently tests for:
> > 
> > ```
> > using P = std::unique_ptr<const T, Deleter const&>;
> > static_assert(std::is_nothrow_assignable<P, P&&>::value, "");
> > ```
> > 
> > See [here](https://github.com/llvm-mirror/libcxx/blob/master/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp#L223-L224). When I take Zoe's patch and use `class _Dummy = _Dp` instead of `class _Dummy = typename remove_const<typename remove_reference<_Dp>::type>::type`, this test breaks (and others too):
> > 
> > ```
> > <snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:224:5: error: static_assert failed due to requirement 'std::is_nothrow_assignable<std::__1::unique_ptr<const A, const GenericConvertingDeleter<0> &>, std::__1::unique_ptr<A, const GenericConvertingDeleter<0> &> &&>::value' ""
> >     static_assert(std::is_nothrow_assignable<U1C, U1&&>::value, "");
> >     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > <snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:410:5: note: in instantiation of function template specialization 'test_sfinae<false>' requested here
> >     test_sfinae</*IsArray*/false>();
> >     ^
> > <snip>/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move_convert.pass.cpp:301:5: error: static_assert failed due to requirement 'std::is_nothrow_assignable<std::__1::unique_ptr<const A, NCDeleter<const A> &>, std::__1::unique_ptr<A, NCDeleter<const A> &> >::value' ""
> >     static_assert(std::is_nothrow_assignable<APtr, BPtr>::value, "");
> >     ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ```
> > 
> > Given that, my question was:
> > 
> > 1. Why do we test for that? Does the Standard say it should be valid? I thought this was the case, but it isn't (see below).
> > 2. If the Standard says it's valid, then was it discussed in LWG that [LWG2899](https://cplusplus.github.io/LWG/issue2899)'s resolution would break that use case?
> > 
> > 
> > ### Why I don't think `std::declval<unique_ptr<T, Deleter const&>&>() = std::declval<unique_ptr<T, Deleter const&>&&>()` is valid according to the Standard
> > 
> > 1. Deleters are allowed to be lvalue references to function object types per [unique_ptr.single#1](http://eel.is/c++draft/unique.ptr.single#1). And a `Deleter const` is a valid function object type if `Deleter`'s `operator()` is `const`-qualified, per [function.objects#1](http://eel.is/c++draft/function.objects#1). So `Deleter const&` is a valid template argument for `unique_ptr`'s deleter.
> > 2. However, per [unique.ptr.single/2](http://eel.is/c++draft/unique.ptr.single#asgn-2):
> >     "Otherwise, `D` is a reference type; `remove_reference_t<D>` shall meet the `Cpp17CopyAssignable` requirements and assignment of the deleter from an lvalue of type `D` shall not throw an exception."
> >     Since `remove_reference_t<D>` is `Deleter const`, it's not `Cpp17CopyAssignable` and the requirements are not met.
> > 
> > So our tests are wrong, they shouldn't expect move-assignment of a `unique_ptr` with a `Deleter const&` (or a `Deleter const`, for that matter) to be valid.
> > 
> In other words, my concern was that LWG2899's resolution would break valid code. But, if my analysis above is correct, that code (move-assigning `unique_ptr` with `Deleter const&`) was already invalid. So LWG2899's resolution doesn't break valid code: it causes already invalid code to break in a different way than previously, and that happens to break our tests (because they are incorrect).
Thank you for the message, @ldionne. I talked with @mclow.lists a bit about this yesterday and came to the same conclusion, `std::declval<unique_ptr<T, Deleter const&>&>() = std::declval<unique_ptr<T, Deleter const&>&&>()` has never been valid. Even before this patch, the above line would fail to compile, however, 
```
using P = std::unique_ptr<const T, Deleter const&>;
static_assert(std::is_nothrow_assignable<P, P&&>::value, "");
```
Would not. This patch makes sure that `is_assignable<...>`corrisponds correctly to whether that type may be assigned. Our tests are incorrect; people's code may also be incorrect, but, it is more important that the type trait is correct than the possibility we break someone's code. 

With that said, I plan to update both our tests and the `enable_if` statement (though, for a different reason than I stated in my last comment). 

Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66262