[libcxx-commits] [PATCH] D62641: Support arrays in make_shared and allocate_shared (P0674R1)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:1007-1008
+        // [1]: https://en.wikipedia.org/wiki/Data_structure_alignment#Computing_padding
+        size_t __bytes = __elements <= 1 ? sizeof(__unbounded_array_control_block)
+                                         : (__elements - 1) * sizeof(_Tp) + sizeof(__unbounded_array_control_block);
+        constexpr size_t __align = alignof(_Tp);
----------------
Quuxplusone wrote:
> The condition could be `__elements == 0 ? ~~~`; the only singularity requiring a special case is at zero. It looks to me as if `make_shared<int[]>(0)` is intended to be UB: https://eel.is/c++draft/util.smartptr.shared#create-13 — but maybe we want to support it anyway for QoI, or maybe I'm misunderstanding the possibility of returning a pointer to an object of type `U[0]`.
I actually didn't find anything explicitly prohibiting `make_shared<T[]>(0)`. Furthermore, I had never really thought about it, but based on https://stackoverflow.com/a/1087066/627587 it seems like dynamically-sized arrays of size 0 are definitely a thing and they are supported. Arrays with a static size 0 are not, though.

So yeah, I think `make_shared<int[]>(0)` & friends definitely need to work, and it must allocate an array with no members. I realized after reading this that I did have tests for this case already. I will add tests to ensure that we reject arrays with a static size of 0.

I reworded the comment and changed the condition to `__elements == 0`.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:358-360
+template<class _Alloc, class _BidirIter, class = __enable_if_t<
+    __is_cpp17_bidirectional_iterator<_BidirIter>::value
+>>
----------------
Quuxplusone wrote:
> Why does this need to work with non-pointers at all? I'm 99% positive it should take `_Tp*` instead of an arbitrary `_BidirIter`. Likewise throughout — there are a lot of these unnecessarily bidirectional-iterator helpers.
> Likewise line 363: `using _ValueType = _Tp` and then there's no more need for `_ValueType` to exist.
I don't see the benefit in removing abstraction when the cost of doing it is close to nothing. During development of the algorithm, abstracting to arbitrary iterators was incredibly useful, because at some point I was really confused -- these algorithms handle arrays specially, and arrays decay into pointers. Drawing a clear distinction between the iterator and the value type (which is sometimes an array) was very useful, and in fact there were some subtle bugs in the implementation before I decided to write it this way. So I'm pretty attached to the specific way those are written.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62641