[libcxx-commits] [PATCH] D91201: [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/memory:2626
+    using _CompressedPair = __compressed_pair<_Alloc, _Tp>;
+    struct _ALIGNAS_TYPE(_CompressedPair) _Storage {
+        char __blob_[sizeof(_CompressedPair)];
----------------
zoecarver wrote:
> Alternatively, you could make `__blob_` a `std::aligned_storage`. 
I used to love `aligned_storage`, and then I read http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1413r2.pdf. I'm not sure I agree entirely with the paper, but I've been moving away from the type since it seemed like there were efforts to deprecate it. I'm not sure where those efforts ended up, though.


================
Comment at: libcxx/include/memory:2630
+        _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) {
+            ::new ((void*)__get_alloc()) _Alloc(_VSTD::move(__a));
+        }
----------------
zoecarver wrote:
> (This is a new comment, I'm not replying to an old comment, even though that's what it looks like.)
> 
> Isn't this UB? `__blob_` is uninitialized when you call `.first()` on it in `__get_alloc()`.
Yes, it is indeed UB (hopefully benign UB). I ran it through ubsan before updating the patch and it didn't complain, so I thought to myself "it's good enough". But you're entirely right, it *is* plain and simple UB.

The issue is that I haven't been able to find a way to implement it without that. Simply, what we want is this:

```
_Alloc* __get_alloc() _NOEXCEPT {
    std::ptrdiff_t __offset = _CompressedPair::__first_offset(); // how to compute this?
    _Alloc* __alloc = static_cast<_Alloc*>(&__blob_ + __offset);
    return __alloc;
}
```

I wasn't able to find an implementation that didn't actually require an object to be alive:

```
_LIBCPP_INLINE_VISIBILITY
static std::ptrdiff_t __first_offset() _NOEXCEPT {
    __compressed_pair __p; // also requires default-constructing, which doesn't work
    _T1 const* __first = _VSTD::addressof(__p.first());
    return static_cast<char const*>(__first) - static_cast<char const*>(&__p);
}
```

I tried something with `offsetof` inside `__compressed_pair_elem`, but I couldn't compute the offset of the second element of the compressed pair. I don't know whether it's impossible or whether I'm just not imaginative enough. If we can't think of something, I can go back to the `no_unique_address` implementation (it's just a bit more complicated, but it's correct AFAICT).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201