implement
- make_unique_for_overwrite
- make_shared_for_overwrite
- allocate_shared_for_overwrite
These functions are almost the same as make_unique, make_shared, allocate_shared with no arguments except that they don't value initialise the objects
Details
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG9af9d39a47dc: [libc++] implement P1020R1 P1973R1 make_unique[shared]_for_overwrite
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I only skimmed over the patch, can you update the description what it exactly does.
FYI when you create a patch with arc you can use --draft to mark it as draft, then it will be tested by the CI but not show in the review queue.
| libcxx/include/memory | ||
|---|---|---|
| 735 | Does this complete P1020? | |
| libcxx/include/__memory/shared_ptr.h | ||
|---|---|---|
| 264 | Thinking about it some more, I might suggest using something like __default_initialize_tag or __default_construct_tag? That would be more descriptive when you write e.g. explicit __shared_ptr_emplace(__default_initialize_tag, _Alloc __a) { ... } | |
| 1018 | ||
| 1110 | ||
| 1158–1159 | Maybe you could add a comment like // bounded array variants and then // unbounded array variants below just to clarify the organization. | |
| 1181–1186 | This is a nitpick, but I would move this function below template<class _Tp, class _Alloc, class = __enable_if_t<is_bounded_array<_Tp>::value>> _LIBCPP_HIDE_FROM_ABI shared_ptr<_Tp> allocate_shared(const _Alloc& __a, const remove_extent_t<_Tp>& __u) I think the way these were initially organized was first the 2 functions for bounded arrays and then the 2 functions for unbounded arrays. | |
| 1238–1243 | Same comment here, let's move this after make_shared for bounded arrays. | |
| libcxx/include/memory | ||
| 736 | We should also update the release notes. | |
| libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared_for_overwrite.pass.cpp | ||
| 2 | To test these, one thing I might do is use an allocator that reuses an existing buffer that you control. Set up the buffer with some integer pattern in it, then call foo_for_overwrite(), and check that the buffer hasn't changed (in particular hasn't been 0'd out, which would happen if we called new (p) T() instead of new (p) T). | |
| libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.pass.cpp | ||
| 106 | ||
| 111–112 | I don't feel we gain much from auto here. I think this is testing two things at once, and we should have two tests instead. One for checking that the in-class operator new is called, and then another test to check that we don't value-initialize the underlying memory when we call make_unique_for_overwrite. WDYT? | |
| 125 | To test that we're not value initializing, I would suggest overriding the global operator new with one that always returns 0xDEADBEEF'd memory (or whatever pattern you fancy). Then you can check that the memory after calling xxx_for_overwrite is still 0xDEADBEEF. | |
Comment Actions
Per https://lists.isocpp.org/lib/2018/11/9316.php allocate_shared_for_overwrite should not call allocator_traits::destroy, which I think this implementation does?
Comment Actions
It should call the destructor directly, not go through the allocator (since the objects aren't constructed using the allocator).
I need to file an issue to fix the standard.
Comment Actions
Ah, yes, I just re-read the e-mail you linked and I get it. I think we already implement LWG 3008 in its current form, but we wouldn't after your fix. Thanks for the heads up, I'll fix this and backport to LLVM 16.
This doesn't need to be guarded since it's not public API.