Without this, unique_ptr is not constant initialized. I've added a test to that extent.
Unfortunately, I believe there's additional work required for C++11, since unique_ptr
uses std::forward internally which is not constexpr until C++14. Doing something about
that is not part of this patch. unique_ptr not being constant initialized is particularly problematic
because it is in libstdc++, so there's applications that rely on this behavior.
Details
Details
- Reviewers
mclow.lists EricWF
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
This all looks good to me - but with a couple nits.
| include/memory | ||
|---|---|---|
| 2137 | Have you tested this on C++11? | |
| test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp | ||
| 16 | Shouldn't this test be in the libcxx hierarchy, since it uses internal libcxx features (i.e, _LIBCPP_SAFE_STATIC)? | |
| test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp | ||
|---|---|---|
| 19–23 | This is a bit painful to test without _LIBCPP_SAFE_STATIC, because the contents of a will be all 0 bits on program startup regardless of whether static or dynamic initialization is performed. Something like this might work though: extern std::unique_ptr<int> a;
void *trample = std::memset(&a, 0xab, sizeof(a));
_LIBCPP_SAFE_STATIC std::unique_ptr<int> a;
int main() {
// Check that the initialization of 'a' was performed before the initialization of 'trample'.
for (size_t n = 0; n != sizeof(a); ++n)
assert(reinterpret_cast<char*>(trample)[n] == 0xab);
// Put a unique_ptr object back so that the global dtor is valid.
new (&a) std::unique_ptr<int>;
} | |
| include/memory | ||
|---|---|---|
| 2137 | Well, as I mentioned there are other problems on C++11, namely forward not being constexpr (so even the simple constructors below can't be constant initialized in C++11). I would REALLY like for unique_ptr to be constant initialized even in C++11 mode. I feel like arguably the standard requires it by marking the constructor constexpr. | |
| test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp | ||
| 16 | I saw this feature was already used in the std/ hierarchy in rL280777, so I thought it was ok, but happy to do whatever you think is best. | |
| 19–23 | Sounds good. | |
Comment Actions
Small bump here. Is it possible to rewrite these without std::forward, so we can have the const-initialization behavior on C++11 (or is there are different solution?).
| include/memory | ||
|---|---|---|
| 2137 | One way to deal with that is to define a call __forward() that is the same as forward() except that it's constexpr in C++11, and have compressed_pair call that. | |
| include/memory | ||
|---|---|---|
| 2137 | I was worried that something in the definition of forward required C++14 constexpr semantics? Is that not the case. If so this sounds like a good solution, and I can update the patch accordingly. | |
Comment Actions
Add private copy of forward (__forward) that is constexpr even in C++11 mode, use test suggested by @rsmith
Comment Actions
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.
| include/memory | ||
|---|---|---|
| 2056 | I would put this right next to std::forward. | |
| 2511 | 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. | |
| 2526 | 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). | |
| test/std/utilities/memory/unique.ptr/unique.ptr.runtime/unique.ptr.runtime.ctor/constinit.pass.cpp | ||
| 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. | |
| 21 | 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. | |
Comment Actions
FYI std::move and std::forward are now constexpr in C++11. There is no need to use __forward.
Comment Actions
back to _VSTD::forward, address review comments, add tests for non-empty deleter and unique_ptrs of arrays.
Comment Actions
Before I commit this, I just realized
__libcpp_compressed_pair_imp& operator=(const __libcpp_compressed_pair_imp& __p)
_NOEXCEPT_(is_nothrow_copy_assignable<_T1>::value &&
is_nothrow_copy_assignable<_T2>::value)
{
_T1::operator=(__p.first());
__second_ = __p.second();
return *this;
}is only constexpr in C++14, right?
Comment Actions
Some extra _AFTER_CXX11 that may be necessary - please double check me here, C++11 constexpr rules are not exactly my specialty ;).
Comment Actions
Bump, please do take another close look at the latest update, particularly with respecting to marking things constexpr that are not so in C++11.
I would put this right next to std::forward.