[libcxx-commits] [PATCH] D62760: allocate_shared should call allocator_traits::construct

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 4 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/memory:3658
     __data_.second().~_Alloc();
-    __a.deallocate(_PTraits::pointer_to(*this), 1);
+    __a.deallocate(_PTraits::pointer_to(*reinterpret_cast<char*>(this)), __size);
 }
----------------
ldionne wrote:
> Can you explain why, in this patch, using `__a.deallocate(_PTraits::pointer_to(*this), 1);` is not sufficient?
Because `_Al` is rebound to `char` type and used in `_ATraits` which is used to get the pointer type for `_PTraits` so, `_PTraits:: pointer_to ` will be expecting a `char*`.


================
Comment at: libcxx/include/memory:4542
 {
-    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
+//    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
 
----------------
ldionne wrote:
> This should either be there, or not be there. We don't leave commented-out stuff in the code :-)
Oops. I thought I removed this. 


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:16
+
+// This patch tests that allocator_traits::construct is used in allocate_shared.
+
----------------
ldionne wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > Please explain why this needs to be tested in the comment. It could be as simple as "[...] as requested in C++20", which is the case I think.
> > Huh, you're right. I didn't realize this was since C++20. I'll add the comment. 
> Is this a DR that was back-ported to older standards, or should this "feature" only work in C++20? If so, I would expect something like `UNSUPPORTED: c++03, c++11, c++14, c++17`.
[[ https://en.cppreference.com/w/cpp/memory/shared_ptr/allocate_shared | According to cppreference ]], it should use placement new before C++20 (FWIW that seems a bit odd to me). I'll update this to use placement new before C++20.


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp:109
+{
+    // TODO: in C++03 mode construct only works for some variadics.
+#if TEST_STD_VER >= 11
----------------
ldionne wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > Please fix this `todo`. It's fine to assume that C++03 has variadics, cause we only support C++03 with Clang, which supports variadics.
> > Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change. 
> I guess my point is that you can always assume variadics, even in C++03 mode. We can remove any attempt to emulate variadics in C++03 prior to applying this patch.
OK good to know. I'll submit a patch for that today. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62760