[libc++] Value-initialize unique_ptr's deleter_type

[bergjohan]

Summary

According to the C++ standard, the stored pointer and the stored deleter
should be value-initialized.

[ldionne]

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

libcxx/include/__memory/unique_ptr.h

187

We have the same problem here AFAICT.

411

And here!

[bergjohan]

Add missing value-initializations

[bergjohan]

In D113612#3140624, @ldionne wrote:

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

Wait, scratch that, the auto_ptr constructor should be disable with a non-default deleter, so I guess it doesn't really make any difference.

[ldionne]

In D113612#3141239, @bergjohan wrote:

In D113612#3140624, @ldionne wrote:

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

It looks like the auto_ptr test requires C++11 or C++14. You can see how to run the test suite with different parameters here: https://libcxx.llvm.org/TestingLibcxx.html#usage

Long story short, you can do:

<build-dir>/bin/llvm-lit -sv libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/auto_pointer.pass.cpp --param std=c++14

[bergjohan]

Value-initialize in auto_ptr constructor

[bergjohan]

In D113612#3142773, @ldionne wrote:

In D113612#3141239, @bergjohan wrote:

In D113612#3140624, @ldionne wrote:

Thanks for the patch! Is this implementing a specific LWG issue/paper, or did you just notice that we differed from the spec? I looked but couldn't find anything.

Thanks for the review! I just noticed the diff from the spec.

I can't believe I missed those two cases. I made the changes and added another test case.

I also noticed there's a missing value initialization in the constructor taking an auto_ptr. But I was not able to run the auto_pointer.pass.cpp test because it was marked as unsupported.
Should we update that constructor as well, and if so, how do I run the test?

It looks like the auto_ptr test requires C++11 or C++14. You can see how to run the test suite with different parameters here: https://libcxx.llvm.org/TestingLibcxx.html#usage

Long story short, you can do:

<build-dir>/bin/llvm-lit -sv libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/auto_pointer.pass.cpp --param std=c++14

Thanks, that did the trick!

I've added value-initialization to the auto_ptr constructor to match the spec, however, I'm not able to add a test for this specific case since this constructor is disabled for non-default deleters. The current tests for the auto_ptr constructor are still passing after the change.

[ldionne]

Can you please rebase onto main? That will re-trigger CI and it should pass.

Also, if you want me to commit this for you, please provide Author Name <email@domain>. Thanks!

[bergjohan]

Rebase onto main

[bergjohan]

In D113612#3146758, @ldionne wrote:

Can you please rebase onto main? That will re-trigger CI and it should pass.

Also, if you want me to commit this for you, please provide Author Name <email@domain>. Thanks!

Done!

Thanks, you can use "Johan Berg <[e-mail]>

libcxx/include/__memory/unique_ptr.h

private:
   using _EnableIfDeleterAssignable = typename enable_if<
       is_assignable<_Dp&, _UDel&&>::value
     >::type;
 
 public:
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(pointer(), __default_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(pointer(), __default_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  explicit unique_ptr(pointer __p) _NOEXCEPT : __ptr_(__p, __default_init_tag()) {}
+  explicit unique_ptr(pointer __p) _NOEXCEPT : __ptr_(__p, __value_init_tag()) {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterConstructible<_LValRefType<_Dummy> > >
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr(pointer __p, _LValRefType<_Dummy> __d) _NOEXCEPT
       : __ptr_(__p, __d) {}
 
   template <bool _Dummy = true,

 
 #if _LIBCPP_STD_VER <= 14 || defined(_LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR)
   template <class _Up>
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr(auto_ptr<_Up>&& __p,
              typename enable_if<is_convertible<_Up*, _Tp*>::value &&
                                     is_same<_Dp, default_delete<_Tp> >::value,
                                 __nat>::type = __nat()) _NOEXCEPT
-      : __ptr_(__p.release(), __default_init_tag()) {}
+      : __ptr_(__p.release(), __value_init_tag()) {}
 #endif
 
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr& operator=(unique_ptr&& __u) _NOEXCEPT {
     reset(__u.release());
     __ptr_.second() = _VSTD::forward<deleter_type>(__u.get_deleter());
     return *this;
   }
private:
   using _EnableIfDeleterAssignable _LIBCPP_NODEBUG = typename enable_if<
       is_assignable<_Dp&, _UDel&&>::value
     >::type;
 
 public:
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(pointer(), __default_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr() _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
 
   template <bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy> >
   _LIBCPP_INLINE_VISIBILITY
-  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(pointer(), __default_init_tag()) {}
+  _LIBCPP_CONSTEXPR unique_ptr(nullptr_t) _NOEXCEPT : __ptr_(__value_init_tag(), __value_init_tag()) {}
 
   template <class _Pp, bool _Dummy = true,
             class = _EnableIfDeleterDefaultConstructible<_Dummy>,
             class = _EnableIfPointerConvertible<_Pp> >
   _LIBCPP_INLINE_VISIBILITY
   explicit unique_ptr(_Pp __p) _NOEXCEPT
-      : __ptr_(__p, __default_init_tag()) {}
+      : __ptr_(__p, __value_init_tag()) {}
 
   template <class _Pp, bool _Dummy = true,
             class = _EnableIfDeleterConstructible<_LValRefType<_Dummy> >,
             class = _EnableIfPointerConvertible<_Pp> >
   _LIBCPP_INLINE_VISIBILITY
   unique_ptr(_Pp __p, _LValRefType<_Dummy> __d) _NOEXCEPT
       : __ptr_(__p, __d) {}
 

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/default.pass.cpp

#endif
   }
   {
     std::unique_ptr<ElemType, NCDeleter<ElemType> > p;
     assert(p.get() == 0);
     assert(p.get_deleter().state() == 0);
     p.get_deleter().set_state(5);
     assert(p.get_deleter().state() == 5);
   }
+  {
+    std::unique_ptr<ElemType, DefaultCtorDeleter<ElemType> > p;
+    assert(p.get() == 0);
+    assert(p.get_deleter().state() == 0);
+  }
 }
 
 DEFINE_AND_RUN_IS_INCOMPLETE_TEST({
   doIncompleteTypeTest(0);
   doIncompleteTypeTest<IncompleteType, Deleter<IncompleteType> >(0);
 } {
   doIncompleteTypeTest<IncompleteType[]>(0);
   doIncompleteTypeTest<IncompleteType[], Deleter<IncompleteType[]> >(0);

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/nullptr.pass.cpp

#endif
     std::unique_ptr<VT> p(nullptr);
     assert(p.get() == 0);
   }
   {
     std::unique_ptr<VT, NCDeleter<VT> > p(nullptr);
     assert(p.get() == 0);
     assert(p.get_deleter().state() == 0);
   }
+  {
+    std::unique_ptr<VT, DefaultCtorDeleter<VT> > p(nullptr);
+    assert(p.get() == 0);
+    assert(p.get_deleter().state() == 0);
+  }
 }
 
 template <class VT>
 void test_sfinae() {
 #if TEST_STD_VER >= 11
   { // the constructor does not participate in overload resolution when
     // the deleter is a pointer type
     using U = std::unique_ptr<VT, void (*)(void*)>;

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/pointer.pass.cpp

#endif
   {
     A* p = newValue<ValueT>(expect_alive);
     assert(A::count == expect_alive);
     std::unique_ptr<ValueT, NCDeleter<ValueT> > s(p);
     assert(s.get() == p);
     assert(s.get_deleter().state() == 0);
   }
   assert(A::count == 0);
+  {
+    A* p = newValue<ValueT>(expect_alive);
+    assert(A::count == expect_alive);
+    std::unique_ptr<ValueT, DefaultCtorDeleter<ValueT> > s(p);
+    assert(s.get() == p);
+    assert(s.get_deleter().state() == 0);
+  }
+  assert(A::count == 0);
 }
 
 void test_derived() {
   {
     B* p = new B;
     assert(A::count == 1);
     assert(B::count == 1);
     std::unique_ptr<A> s(p);

libcxx/test/support/deleter_types.h

 private:
     template <class U>
         PointerDeleter(const PointerDeleter<U, ID>&,
             typename std::enable_if<!std::is_same<U, T>::value>::type* = 0);
 };
 
 #endif // TEST_STD_VER >= 11
 
+template <class T>
+class DefaultCtorDeleter
+{
+    int state_;
+
+public:
+    int state() const {return state_;}
+    void operator()(T* p) {delete p;}
+};
+
+template <class T>
+class DefaultCtorDeleter<T[]>
+{
+    int state_;
+
+public:
+    int state() const {return state_;}
+    void operator()(T* p) {delete [] p;}
+};
+
 #endif // SUPPORT_DELETER_TYPES_H