[PATCH] D24372: [libcxx] Sprinkle constexpr over compressed_pair

[Eric Fiselier via cfe-commits]

EricWF added a comment.

Thanks for working on this. That static initialization bug is ugly.

My only concern with this patch is that adding `constexpr` to unconstrained constructors is going to result compile errors caused by Clang's eager instantiation.

Also, unlike @mclow.lists, I think we should make `std::forward` and `std::move` constexpr in C++11 as an extension. libstdc++ does it so it should be safe. I'm working on a patch that adds this extension now.

More comments to come shortly.


================
Comment at: include/memory:2056
@@ -2055,1 +2055,3 @@
 
+// Private copy of forward, for use only in this file, which is constexpr even
+// in C++11
----------------
I would put this right next to `std::forward`.

================
Comment at: include/memory:2531
@@ -2491,3 +2530,3 @@
 
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
     __compressed_pair& operator=(__compressed_pair&& __p)
----------------
The body of this function is only constexpr in C++14 because it contains two statements.

`return base::operator=(static_cast<__compressed_pair&&>(__p)), *this` is a constant expression in C++11 though.

================
Comment at: include/memory:2546
@@ -2507,2 +2545,3 @@
+        _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
         __compressed_pair(piecewise_construct_t __pc, tuple<_Args1...> __first_args,
                                                       tuple<_Args2...> __second_args)
----------------
This isn't a constant expression until C++14, since both `tuple` and `move` are only constexpr in C++14.

(Although I guess two default constructed tuple's would be constexpr in C++11).

================
Comment at: test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp:1
@@ +1,2 @@
+//===----------------------------------------------------------------------===//
+//
----------------
This test should live under `test/std/utilities/memory/unique.ptr/unique.ptr.single/unique.ptr.single.ctor` since `unique.ptr.runtime` is for unique_ptr's of arrays (e.g. `unique_ptr<T[]>`). Adding a separate version of this test for unique_ptr<T[]> would also be appreciated.

================
Comment at: test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp:20
@@ +19,3 @@
+extern std::unique_ptr<int> a;
+extern std::unique_ptr<int> b;
+void *tramplea = std::memset(&a, 0xab, sizeof(a));
----------------
Could this add a test case for a unique_ptr with a non-empty deleter? That will test an entirely different specialization of `__compressed_pair`.



Repository:
  rL LLVM

https://reviews.llvm.org/D24372