[libcxx-commits] [PATCH] D140913: [In Progress] [libc++] make_unique[shared]_for_overwrite

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__memory/shared_ptr.h:264
+#if _LIBCPP_STD_VER >= 20
+struct __shared_ptr_overwrite_tag{};
+#endif
----------------
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) { ... }
```


================
Comment at: libcxx/include/__memory/shared_ptr.h:1020
+    {
+        std::uninitialized_default_construct_n(std::begin(__data_), __count_);
+    }
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:1110
+    explicit __bounded_array_control_block(_Alloc const& __alloc, __shared_ptr_overwrite_tag) : __alloc_(__alloc) {
+        std::uninitialized_default_construct_n(std::addressof(__data_[0]), _Count);
+    }
----------------



================
Comment at: libcxx/include/__memory/shared_ptr.h:1156
 #if _LIBCPP_STD_VER > 17
 
 template<class _Tp, class _Alloc, class = __enable_if_t<is_bounded_array<_Tp>::value>>
----------------
Maybe you could add a comment like `// bounded array variants` and then `// unbounded array variants` below just to clarify the organization.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1185-1190
+template<class _Tp, class _Alloc, __enable_if_t<is_bounded_array<_Tp>::value, int> = 0>
+_LIBCPP_HIDE_FROM_ABI
+shared_ptr<_Tp> allocate_shared_for_overwrite(const _Alloc& __a)
+{
+    return std::__allocate_shared_bounded_array<_Tp>(__a, __shared_ptr_overwrite_tag{});
+}
----------------
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.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1227-1232
+template<class _Tp, __enable_if_t<is_bounded_array<_Tp>::value, int> = 0>
+_LIBCPP_HIDE_FROM_ABI
+shared_ptr<_Tp> make_shared_for_overwrite()
+{
+    return std::__allocate_shared_bounded_array<_Tp>(allocator<_Tp>(), __shared_ptr_overwrite_tag{});
+}
----------------
Same comment here, let's move this after `make_shared` for bounded arrays.


================
Comment at: libcxx/include/memory:736
+  shared_ptr<T> allocate_shared_for_overwrite(const A& a, size_t N);          // T is U[], C++20
+
 template<class T>
----------------
We should also update the release notes.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared_for_overwrite.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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`).


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.pass.cpp:105
+
+void testCustomNew() {
+  struct AggregateWithCustomNew {
----------------



================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.pass.cpp:110-111
+    static void* operator new(size_t n) {
+      auto ptr     = ::operator new(n);
+      auto charPtr = reinterpret_cast<char*>(ptr);
+      *charPtr     = 'x';
----------------
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?


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.pass.cpp:124
+  };
+
+  // single with custom operator new
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140913