Details
- Reviewers
mclow.lists ldionne - Group Reviewers
Restricted Project - Commits
- rG6a328c66d35c: [libc++] shared_ptr deleter requirements (LWG 2802).
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This patch is missing the main point of LWG 2802, and that is making std::shared_ptr support move-only deleters. You need to make this test that constructors and make functions accepting a deleter accept this code:
template <class ValueT>
struct MyDeleter {
MyDeleter() = delete;
MyDeleter(MyDeleter const&) = delete;
MyDeleter(MyDeleter&&) = default;
explicit MyDeleter(secret_type) {} // so you can construct it for the test.
void operator()(ValueT*);
};Additionally, the static asserts you're adding are required to be SFINAE checks according to the standard.
This was added by LWG 2875 (https://cplusplus.github.io/LWG/lwg-defects.html#2875)
| include/memory | ||
|---|---|---|
| 3719 ↗ | (On Diff #200839) | Does this compile in C++03, C++11, or C++14? |
I rewrote this patch so that it doesn't require my (some, now obsolete) other patches. This patch does now rely on the fix from D69344 for C++03 move-constructibility. Also, I updated the tests.
Don't mark 2875 as complete. That would requires some of my other patches so, I'll fix that issue down the road.
In C++03 __compressed_pair doesn't have an implicit move constructor. So, I disabled the tests in C++03.
- Rebase off main.
| libcxx/include/memory | ||
|---|---|---|
| 3015 | I was just thinking, is this going to be problematic if __d has already been moved? | |
LGTM except for the move issue (which is a really good catch BTW, I hadn't thought about that at all).
| libcxx/include/memory | ||
|---|---|---|
| 3015 | Good observation. I think it's an issue, but it's really easy to fix. First, do we agree that line 3010 can't throw? Second, do we agree that we can assume move constructors don't throw? So we only have to see whether something can throw on line 3009 after the deleter has been moved out of.
None of the stuff inside the implementation of __shared_ptr_pointer::__shared_ptr_pointer is an issue either, since it just moves stuff into the compressed pairs, so the same logic applies. This reasoning applies to all 4 constructors being touched by this patch. So I think we only need to use std::move(__a) and we're safe even from a very pedantic point of view. | |
| libcxx/include/memory | ||
|---|---|---|
| 3015 | Hmm. I think move constructors shouldn't throw, but are allowed to. It's not undefined behavior to have a throwing move constructor (unless I'm mistaken, which is entirely possible). But I think you're right, the only thing we're really worried about is the allocator throwing. So what if we just put the call to .allocate inside of the try/catch? I'll update this patch to do that. | |
| libcxx/include/memory | ||
|---|---|---|
| 3015 | Yes, they are allowed to throw, but in that case we don't provide the strong exception guarantee, i.e. the code already behaves correctly. From cppreference:
But anyway, scratch this discussion. I checked the requirements for this constructor, and it requires that constructing the deleter doesn't throw an exception. Furthermore, Allocators are always required to not throw exceptions when copied or moved. So there's no issue here, and the code is correct as-is. We can still use std::move(__a) for "efficiency", but there's not difference in correctness. | |
Oops, and it does seem like this fails in C++03 mode - so please fix that C++03 failure but then you can commit. Thanks!
I'll fix the C++03 failures.
| libcxx/include/memory | ||
|---|---|---|
| 3015 | OK. Thanks for checking :) I think using std::move on __a is a separate patch. | |
Ignore this, it's part of D69344.