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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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)];
----------------
CaseyCarter wrote:
> 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).
Thanks for the info!


================
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;
----------------
CaseyCarter wrote:
> 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).
I guess you are right. IMO this should be a libc++-specific test then, since it tests implementation details which are unspecified in the standard.


================
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)
   {
----------------
CaseyCarter wrote:
> 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.)
Yes, I think this should be libc++ specific then.


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

https://reviews.llvm.org/D141004