[libcxx-commits] [PATCH] D131315: [libc++] Implement P2273R3 (`constexpr` `unique_ptr`)

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general LGTM but several minor issue.
When you update the patch please do *not* rebase. That makes it a lot easier to just view the diff.



================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:47
 "`P2255R3 <https://wg21.link/P2255R3>`__","LWG","A type trait to detect reference binding to temporary","February 2022","",""
-"`P2273R3 <https://wg21.link/P2273R3>`__","LWG","Making ``std::unique_ptr`` constexpr","February 2022","",""
+"`P2273R3 <https://wg21.link/P2273R3>`__","LWG","Making ``std::unique_ptr`` constexpr","February 2022","|Complete|","16.0"
 "`P2387R3 <https://wg21.link/P2387R3>`__","LWG","Pipe support for user-defined range adaptors","February 2022","",""
----------------
Can you add a note that `make_unique_for_overwrite` isn't done yet since P1020 hasn't been implemented yet.


================
Comment at: libcxx/include/memory:437
+    constexpr unique_ptr(unique_ptr&& u) noexcept;               // constexpr since C++23
+    constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
     template <class U, class E>
----------------
Nice catch!


================
Comment at: libcxx/include/memory:479
+    constexpr unique_ptr(pointer p, see below d) noexcept;      // constexpr since C++23
+    constexpr unique_ptr(unique_ptr&& u) noexcept;              // constexpr since C++23
+    constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { } // constexpr since C++23
----------------
It seems
```
 template<class U, class E>
   constexpr unique_ptr(unique_ptr<U, E>&& u) noexcept;
```
is missing. In general this section seems to already miss `template<class U>`.


================
Comment at: libcxx/include/memory:486
     // assignment
-    unique_ptr& operator=(unique_ptr&& u) noexcept;
-    unique_ptr& operator=(nullptr_t) noexcept;
+    constexpr unique_ptr& operator=(unique_ptr&& u) noexcept;   // constexpr since C++23
+    constexpr unique_ptr& operator=(nullptr_t) noexcept;        // constexpr since C++23
----------------
Pre existing issue: here too seems to be a member to be missing. 


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/nullptr.pass.cpp:13
 
 // unique_ptr(nullptr_t);
 
----------------
Please update the synopsis. Also validate the other tests please.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/reset_self.pass.cpp:16
 #include <memory>
+#include <cassert>
 
----------------
There's no `assert` added.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp:21
 class A {
   int state_;
   static int next_;
----------------



================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/op_subscript.runtime.pass.cpp:26-33
+#if TEST_STD_VER >= 23
+    if (TEST_IS_CONSTANT_EVALUATED) {
+      state_ = 0;
+    } else
+#endif
+    {
+      state_ = ++next_;
----------------
Combined with setting the default value to 0 this seems easier to read.
Before C++23 the code will never be constant evaluated.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/convert_ctor.pass.cpp:42
+  static_assert(test());
+#endif
 }
----------------
Please add `return 0;`.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt/default.pass.cpp:37
+  static_assert(test());
+#endif
 }
----------------
Please add `return 0;`.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/convert_ctor.pass.cpp:36
+  static_assert(test());
+#endif
 }
----------------
Please add `return 0;`.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.dltr/unique.ptr.dltr.dflt1/default.pass.cpp:39
+  static_assert(test());
+#endif
 }
----------------
Please add `return 0;`.


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

https://reviews.llvm.org/D131315