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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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);
 }
----------------
Can you explain why, in this patch, using `__a.deallocate(_PTraits::pointer_to(*this), 1);` is not sufficient?


================
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");
 
----------------
This should either be there, or not be there. We don't leave commented-out stuff in the code :-)


================
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.
+
----------------
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`.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62760