[libcxx-commits] [PATCH] D141004: [libc++][test] Port memory_resource tests to MSVCSTL

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp:41
   std::pmr::polymorphic_allocator<P> A(M);
-  P* ptr  = (P*)std::malloc(sizeof(P));
-  P* ptr2 = (P*)std::malloc(sizeof(P));
+  P* ptr  = (P*)new char[sizeof(P)];
+  P* ptr2 = (P*)new char[sizeof(P)];
----------------
Does this change alignment guarantees?


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/resource.pass.cpp:36
   }
+#ifdef _LIBCPP_VERSION
   {
----------------
I think this should be removed and we should add a `_LIBCPP_ASSERT(__r != nullptr, "memory_resource pointer has to be non-null");` in the constructor instead.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/select_on_container_copy_construction.pass.cpp:29
+    void* do_allocate(std::size_t, std::size_t) override {
+      abort();
+    }
----------------
Missing `<cstdlib>` include.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/select_on_container_copy_construction.pass.cpp:60
     std::pmr::set_default_resource(mptr);
-    A const a(nullptr);
-    assert(a.resource() == nullptr);
+#ifdef _LIBCPP_VERSION
+    std::pmr::memory_resource* mptr2 = nullptr;
----------------
Why the special-casing for libc++?


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp:46
+#ifdef _MSVC_STL_VERSION
+    // MSVC can't implement LWG-3120 until the next ABI break
+    // (see https://github.com/microsoft/STL/issues/1468).
----------------
Couldn't you just test this downstream and just have `#ifndef _MSVC_STL_VERSION` around the assert?


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_with_initial_size.pass.cpp:38
 int main(int, char**) {
+#ifdef _LIBCPP_VERSION
   test(1);
----------------
What's the problem here?


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.res.pool/mem.res.pool.ctor/ctor_does_not_allocate.pass.cpp:23-26
+#if defined(_MSVC_STL_VERSION) && _ITERATOR_DEBUG_LEVEL == 2
+// MSVCSTL's pool resources use a vector internally, which allocates a tracking
+// object when iterator debugging
+constexpr int alloc_delta = 1;
----------------
AFAICT this is non-conforming and should therefore not be tested here. Instead, this test should be `XFAIL`ed for the problematic case, since it's all that is tested. Same for the cases below.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.res.pool/mem.res.pool.ctor/sync_with_default_resource.pass.cpp:23
   std::pmr::set_default_resource(expected);
+#if !defined(_MSVC_STL_VERSION) || !defined(_DEBUG)
   {
----------------
What is the problem here? Same question below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141004