[libcxx-commits] [PATCH] D93153: [libc++] Consistently replace `::new(__p) T` with `::new ((void*)__p) T`. NFCI

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__functional_03:128
     typedef __allocator_destructor<_Ap> _Dp;
     unique_ptr<__func, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
+    ::new ((void*)__hold.get()) __func(__f_.first(), _Alloc(__a));
----------------
We could use @ldionne's new `__allocation_guard` here :)


================
Comment at: libcxx/include/__functional_03:129
     unique_ptr<__func, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
-    ::new (__hold.get()) __func(__f_.first(), _Alloc(__a));
+    ::new ((void*)__hold.get()) __func(__f_.first(), _Alloc(__a));
     return __hold.release();
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > Nit: would be better to make these `static_cast`s IMHO.
> > Sorry, didn't realize this already landed. No need to change it. 
> No worries, I would have argued anyway. ;)
> My goal is to be consistent on something that we can cut and paste everywhere, so I would want to use `static_cast` here //only if// you are willing to use `static_cast` everywhere.
> 
> For this spot in particular, are you worried that the allocator's pointers might be fancy? If that's the issue, then I think we actually need something like `::new ((void*)_VSTD::addressof(*__hold.get())) __func(...)`. The kind of cast isn't the problem; it's that no such conversion might exist at all. (And my recent patch-series has taught me that `*__hold.get()` avoids ADL when `*__hold` would trigger it. Gross, right?)
My point was exactly that, I'd prefer to see `static_cast` everywhere rather than a C-style cast. But that's mostly just a stylistic thing. I don't have any worry about a cast to `void*` failing at compile time. (Which is why it was a nit.)

I was moving too quickly when I wrote this comment ;) I didn't carefully read where I made the suggestion. I think you're right, we really should be doing `_VSTD::addressof(*__hold.get())` in most places. That's what we do for `allocate_shared`, for example. But that's a different patch...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93153