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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 7 inline comments as done.
zoecarver added a comment.

Sorry about the delay updating this patch. I think I got everything except the additional overloads.

I spent some time trying to add the other overloads. Unless I am not seeing something (which is very possible) I think the `shared_ptr` class might have to have some serious work in order to support array types with undefined length.



================
Comment at: include/memory:3709
+{
+    t.~_Tp();
+}
----------------
ldionne wrote:
> This here should be `_VSTD::destroy_at(&t)`, and `t` needs to be mangled.
Could be wrong about this but I think that this needs to work in C++11 but `destroy` wasn't introduced until C++17. 


================
Comment at: include/memory:3716
+{
+    for (auto i = __n; i > 0; i--) __shared_ptr_emplace__destroy(t[i]);
+}
----------------
ldionne wrote:
> I would use this instead:
> 
> ```
> _VSTD::destroy(_VSTD::begin(t), _VSTD::end(t))
> ```
> 
> Also, `t` needs to be mangled to `__t`.
> 
Same as above comment.


================
Comment at: test/libcxx/memory/shared_ptr_array.pass.cpp:23
+
+struct test_onstructors
+{
----------------
ldionne wrote:
> `test_constructors` (typo)
How did I not see this 🤦‍♂️


================
Comment at: test/libcxx/memory/shared_ptr_array.pass.cpp:52
+    auto pi = &i;
+    ASSERT_SAME_TYPE(decltype(**pi), int);
+     */
----------------
ldionne wrote:
> https://wandbox.org/permlink/YuADEmFtbD8ZlNS3
> 
> The result of dereferencing a pointer like `**pi` is a lvalue (`int&` in this case).
Very helpful thank you. 


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

https://reviews.llvm.org/D57403