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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
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)];
----------------
philnik wrote:
> Does this change alignment guarantees?
It does not. malloc (C 7.24.3) returns a pointer aligned for "any type of object with a fundamental alignment requirement and size less than or equal to the size requested", whereas new char[n] gets storage from operator new[] ([basic.stc.dynamic.allocation]/3.2) which is "aligned for any object that does not have new-extended alignment and is no larger than the request size" (at least as strict a requirement as "fundamental alignment") which it might offset by a multiple of the fundamental alignment requirement ([expr.new]/16).


================
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;
----------------
philnik wrote:
> Why the special-casing for libc++?
Just to preserve the `nullptr` behavior. Since you've suggested above that the constructor should refuse `nullptr` - which I agree is a good idea - I'll remove the special-case here.


================
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).
----------------
philnik wrote:
> Couldn't you just test this downstream and just have `#ifndef _MSVC_STL_VERSION` around the assert?
I want the test to break loudly when we implement LWG-3120, which honestly I could do as well by XFAILing it in our out-of-band control structure. On further thought I believe that's preferable: the cases that do pass here pass for the wrong reasons, so this test would be incredibly fragile for us. I'll revert this part of the change (leaving the comment fix and the change from `assert` to `ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS`).


================
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);
----------------
philnik wrote:
> What's the problem here?
[LWG-3637](https://cplusplus.github.io/LWG/issue3637) will possibly make it a requirement that `allocate` may not return the same pointer twice without intervening deallocation. That requirement can't be satisfied by an implementation that allocates 0 bytes from an exhausted block - as `test(1)` expects - since repeated calls would presumably return the same address. Making this case libc++-only allows implementations like MSVCSTL to pass as well. 


================
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;
----------------
philnik wrote:
> 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.
I believe it is conforming; the Standard gives a lot of leeway with "It is unspecified if, or under what conditions, this constructor calls `upstream->allocate()`" ([mem.res.pool.ctor]/3).


================
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)
   {
----------------
philnik wrote:
> What is the problem here? Same question below.
Same as the previous test: our pool resources allocate on construction when iterator debugging is active, so line 26 and 27 fail horribly. I believe this part of the test is non-portable. Maybe it would be better to make it libc++-specific? (Same response below.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141004