Page MenuHomePhabricator

Constrain tuple/unique_ptr move constructors (2899)
Needs ReviewPublic

Authored by zoecarver on Aug 14 2019, 5:08 PM.

Details

Summary

This patch disables unique_ptr& operator=(unique_ptr&&), unique_ptr(unique_ptr&&), and tuple(tuple&&) unless is_move_constructible_v is true. This patch resolves LWG issue 2899.

Event Timeline

zoecarver created this revision.Aug 14 2019, 5:08 PM
zoecarver marked 3 inline comments as done.Aug 14 2019, 5:13 PM

It did not appear that <tuple> needed to be updated. Correct me if I am wrong.

libcxx/include/memory
2491

The indentation is wrong in this section of the file.

2523

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?

2552

This is unconditionally added because otherwise this overload may be picked when the other overloads are disabled.

ldionne added inline comments.Aug 15 2019, 8:47 AM
libcxx/include/memory
2501

Any reason for preferring this style of enable_if (as a default argument) as opposed to using a default _template_ argument?

2523

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.

zoecarver marked 2 inline comments as done.Aug 15 2019, 9:05 AM
zoecarver added inline comments.
libcxx/include/memory
2501

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.

2523

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.

ldionne added inline comments.Aug 15 2019, 10:42 AM
libcxx/include/memory
2523

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?

zoecarver marked an inline comment as done.Sun, Sep 1, 3:15 PM
zoecarver added inline comments.
libcxx/include/memory
2523

Agreed. @mclow.lists friendly ping. What are your thoughts on this?

mclow.lists added inline comments.Wed, Sep 4, 11:28 AM
libcxx/include/memory
2523

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.

zoecarver marked an inline comment as done.Wed, Sep 4, 5:31 PM
zoecarver added inline comments.
libcxx/include/memory
2523

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.

ldionne added inline comments.Thu, Sep 5, 8:26 AM
libcxx/include/memory
2523

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. 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'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. And a Deleter const is a valid function object type if Deleter's operator() is const-qualified, per function.objects#1. So Deleter const& is a valid template argument for unique_ptr's deleter.
  2. However, per unique.ptr.single/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.

ldionne added inline comments.Thu, Sep 5, 8:29 AM
libcxx/include/memory
2523

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).

zoecarver marked an inline comment as done.Thu, Sep 5, 8:58 AM
zoecarver added inline comments.
libcxx/include/memory
2523

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).

mclow.lists added inline comments.Thu, Sep 5, 10:13 AM
libcxx/include/memory
2523

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.

I concur.

zoecarver updated this revision to Diff 218950.Thu, Sep 5, 10:34 AM
  • use _EnableIf when possible
  • update tests and remove std::remove_ref
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 5, 10:34 AM
mclow.lists added inline comments.Thu, Sep 12, 8:36 AM
libcxx/include/memory
2555

Ok, this is just weird. Everywhere else in libc++, we avoid using = delete in C++03 mode, because it's not a thing.
We even have a macro for it in __config:

#ifdef _LIBCPP_CXX03_LANG
#  define _LIBCPP_EQUAL_DELETE
#else
#  define _LIBCPP_EQUAL_DELETE = delete
#endif

And yet, here (twice!) we only do this for C++03.
@EricWF added this in https://llvm.org/r364161; so I think he should explain.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp
103

We have a type trait for move_assignable.

zoecarver marked an inline comment as done.Thu, Sep 12, 9:02 AM
zoecarver added inline comments.
libcxx/include/memory
2555

Looks like it was just after we dropped support for GCC 4. He was probably just removing some of the C++03 clutter & cleaning up our implementation. But, I'll wait for him to respond.