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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
It did not appear that <tuple> needed to be updated. Correct me if I am wrong.
libcxx/include/memory | ||
---|---|---|
2269 | The indentation is wrong in this section of the file. | |
2301 | 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? | |
2330 | This is unconditionally added because otherwise this overload may be picked when the other overloads are disabled. |
libcxx/include/memory | ||
---|---|---|
2279 | Any reason for preferring this style of enable_if (as a default argument) as opposed to using a default _template_ argument? | |
2301 | 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. |
libcxx/include/memory | ||
---|---|---|
2279 | 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. | |
2301 | 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. |
libcxx/include/memory | ||
---|---|---|
2301 | 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? |
libcxx/include/memory | ||
---|---|---|
2301 | Agreed. @mclow.lists friendly ping. What are your thoughts on this? |
libcxx/include/memory | ||
---|---|---|
2301 | 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. |
libcxx/include/memory | ||
---|---|---|
2301 | 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. |
libcxx/include/memory | ||
---|---|---|
2301 | 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:
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
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. |
libcxx/include/memory | ||
---|---|---|
2301 | 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). |
libcxx/include/memory | ||
---|---|---|
2301 | 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). |
libcxx/include/memory | ||
---|---|---|
2301 |
I concur. |
libcxx/include/memory | ||
---|---|---|
2333 | Ok, this is just weird. Everywhere else in libc++, we avoid using = delete in C++03 mode, because it's not a thing. #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. | |
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp | ||
113 | We have a type trait for move_assignable. |
libcxx/include/memory | ||
---|---|---|
2333 | 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. |
I think this looks good to me, but can you please rebase onto master and update the review? It'll run the CI and we can see if it fails under some configuration. Then I'll re-read the discussion we had to confirm we're good to go, and we can ship it.
libcxx/include/memory | ||
---|---|---|
2333 | Feel free to correct me Eric, but to elaborate on my original comment, I suspect this was just to "clean up our implementation." Looking at the commit, you can see that previously these methods were "deleted" in C++03 mode by declaring them privately. After we dropped GCC C++03 support we were able to use = delete instead which is much more explicit. However, as far as I can tell, those methods weren't deleted in any way in the "normal" implementation, so that's probably why they were only applied to C++03 here. |
While attempting to fix the remaining CI build failures I arrived at a problem and I'm not sure what is the best way to proceed. This is the problem:
- The "trivial_abi" attribute requires a move constructor or a copy constructor.
- The standard requires that unique_ptr's "move constructor" doesn't participate in overload resolution unless a requirement is met. This requires a templated "move constructor" so that the function may be disabled.
- A move constructor is not permitted to be templated. From the standard's point of view, I think it's OK for unique_ptr not to have a move constructor so long as it is move constructible (which would be the case if it has a templated "move constructor").
I am not super familiar with the trivial_abi attribute, so maybe there's some way around this. Any suggestions would be appreciated :)
Oh, so that's the reason for the failures. Basically we're bypassing the trivial_abi attribute altogether.
The only way I can *maybe* foresee fixing this in the library would be to specialize unique_ptr based on whether the deleter is move constructible (i.e. specialize based on whether the move constructor should be enabled or not). Now, we can't do that without breaking ABI (because it would require changing template parameters to add one we can use enable_if on), so we'd have to instead:
- Inherit from a class like __unique_ptr_base which itself does some SFINAE in its template parameters, and has two specializations (one with a move ctor and one without).
- Use using __unique_ptr_base::__unique_ptr_base; inside std::unique_ptr.
I'm not 100% sure that works cause I haven't tried it out, but it might be worth trying out. Otherwise, I believe we'd have to discuss with the compiler folks to see whether it makes sense to change the attribute, or we'd need to drop that extension (but that's pretty bad).
Can you try out my suggestion with the base class? I'm thinking about something like:
template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value> class __unique_ptr_base; // Move constructible version template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value> class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, true> { __compressed_pair<pointer, deleter_type> __ptr_; public: _LIBCPP_INLINE_VISIBILITY __unique_ptr_base(__unique_ptr_base&& __u) _NOEXCEPT { ... } // other constructors... }; // Non move-constructible version template <class _Tp, class _Deleter, bool = is_move_constructible<_Deleter>::value> class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI __unique_ptr_base<_Tp, _Deleter, false> { __compressed_pair<pointer, deleter_type> __ptr_; public: _LIBCPP_INLINE_VISIBILITY __unique_ptr_base(__unique_ptr_base&&) = delete; // other constructors... }; template <class _Tp, class _Dp = default_delete<_Tp> > class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr : public __unique_ptr_base<_Tp, _Dp> { ... using __unique_ptr_base::__unique_ptr_base; };
I'm going to have to revisit this patch in depth, likely after consulting with philnik.
The indentation is wrong in this section of the file.