[libcxx-commits] [PATCH] D57403: Extending make_shared to Support Arrays (Partially) - P0674R1

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This is a good start, but please make sure you implement and test the full paper.



================
Comment at: include/memory:3707
+void
+__shared_ptr_emplace__destroy(_Tp& t)
+{
----------------
You have two underscores between `emplace` and `destroy`.


================
Comment at: include/memory:3709
+{
+    t.~_Tp();
+}
----------------
This here should be `_VSTD::destroy_at(&t)`, and `t` needs to be mangled.


================
Comment at: include/memory:3716
+{
+    for (auto i = __n; i > 0; i--) __shared_ptr_emplace__destroy(t[i]);
+}
----------------
I would use this instead:

```
_VSTD::destroy(_VSTD::begin(t), _VSTD::end(t))
```

Also, `t` needs to be mangled to `__t`.



================
Comment at: include/memory:4717
+shared_ptr<_Tp>
 make_shared(_Args&& ...__args)
 {
----------------
You'll need other overloads as outlined in http://wg21.link/p0674. Please add them in this review, otherwise it's too difficult to tell whether the paper is being implemented correctly across reviews.


================
Comment at: test/libcxx/memory/shared_ptr_array.pass.cpp:23
+
+struct test_onstructors
+{
----------------
`test_constructors` (typo)


================
Comment at: test/libcxx/memory/shared_ptr_array.pass.cpp:52
+    auto pi = &i;
+    ASSERT_SAME_TYPE(decltype(**pi), int);
+     */
----------------
https://wandbox.org/permlink/YuADEmFtbD8ZlNS3

The result of dereferencing a pointer like `**pi` is a lvalue (`int&` in this case).


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

https://reviews.llvm.org/D57403