[libcxx-commits] [PATCH] D89057: Add the C++17 <memory_resource> header (mono-patch)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

Could you add comments in the `TODO.txt` for all the experimental things that can be removed in libc++17 (hopefully)? I think most experimental headers could be removed.



================
Comment at: libcxx/src/memory_resource.cpp:367
+    while (capacity < largest_block_size) {
+        capacity <<= 1;
+        __num_fixed_pools_ += 1;
----------------
It's not a strong feeling, but I'd rather see `*= 2` than `<<= 1`. FWIW it produces the same asm, even at `-O0`


================
Comment at: libcxx/src/memory_resource.cpp:387-388
+            __fixed_pools_[i].__release_ptr(__res_);
+        __res_->deallocate(__fixed_pools_,
+            __num_fixed_pools_ * sizeof(__fixed_pool), alignof(__fixed_pool));
+        __fixed_pools_ = nullptr;
----------------



================
Comment at: libcxx/src/memory_resource.cpp:470-471
+
+bool synchronized_pool_resource::do_is_equal(
+    const memory_resource& other) const noexcept
+{
----------------



================
Comment at: libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp:42-48
+#include <memory_resource>
+#include <type_traits>
+#include <utility>
+#include <tuple>
+#include <cassert>
+#include <cstdlib>
+#include "test_std_memory_resource.h"
----------------
Please reorder the includes


================
Comment at: libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp:56-58
+    T *ptr;
+
+    TestHarness() : ptr(A.allocate(1)) {}
----------------



================
Comment at: libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp:73-74
+struct CountCopies {
+  int count;
+  CountCopies() : count(0) {}
+  CountCopies(CountCopies const& o) : count(o.count + 1) {}
----------------



================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/copy.pass.cpp:26-31
+        static_assert(
+            std::is_copy_constructible<A1>::value, ""
+          );
+        static_assert(
+            std::is_move_constructible<A1>::value, ""
+          );
----------------



================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/other_alloc.pass.cpp:28-39
+        static_assert(
+            std::is_convertible<A1 const &, A2>::value, ""
+          );
+        static_assert(
+            std::is_convertible<A2 const &, A1>::value, ""
+          );
+        static_assert(
----------------



================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair.pass.cpp:42
+        typedef std::pmr::polymorphic_allocator<void> A;
+        P * ptr = (P*)std::malloc(sizeof(P));
+        A a;
----------------
Couldn't we just use a `char ptr[sizeof(P)]`? Seems overkill to `malloc()` for that.


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair_rvalue.pass.cpp:39
+    TestResource R;
+    std::pmr::memory_resource * M = &R;
+    std::pmr::polymorphic_allocator<P> A(M);
----------------
Please choose `PtrT* v` or `PtrT *v`, but not `PtrT * v` for all that is sacred. I don't want to multiply my types!


================
Comment at: libcxx/test/std/utilities/utility/mem.res/mem.res.global/default_resource.pass.cpp:67-71
+        static_assert(noexcept(get_default_resource()),
+                      "get_default_resource() must be noexcept");
+
+        static_assert(noexcept(set_default_resource(nullptr)),
+                      "set_default_resource() must be noexcept");
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89057