[libcxx-commits] [PATCH] D132796: [libc++][PMR] Implement pmr::memory_resource

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:403
   __memory/voidify.h
+  __memory_resource/memory_resource.h
   __mutex_base
----------------
There are no changes to the module map.


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:13
+#include <__config>
+#include <bit>
+#include <cstddef>
----------------
Bit has been added in C++20.


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:27
+class _LIBCPP_TYPE_VIS memory_resource {
+  static constexpr auto __max_align = alignof(max_align_t);
+
----------------
please use `size_t` as in the Standard.


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:34
+
+  [[using __gnu__: __returns_nonnull__,
+    __alloc_size__(2),
----------------
Please add some comment or a link to the documentation of this non-standard attribute.


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:38
+  allocate(size_t __bytes, size_t __alignment = __max_align) {
+    _LIBCPP_ASSERT(std::popcount(__alignment) == 1, "alignment has to be a power of two");
+    return ::operator new(__bytes, do_allocate(__bytes, __alignment));
----------------
As a side not in C++20 I would have preferred `std::has_single_bit` instead.


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:38
+  allocate(size_t __bytes, size_t __alignment = __max_align) {
+    _LIBCPP_ASSERT(std::popcount(__alignment) == 1, "alignment has to be a power of two");
+    return ::operator new(__bytes, do_allocate(__bytes, __alignment));
----------------
Mordante wrote:
> As a side not in C++20 I would have preferred `std::has_single_bit` instead.
Can you add a test that triggers this `_LIBCPP_ASSERT`?


================
Comment at: libcxx/include/__memory_resource/memory_resource.h:57
+_LIBCPP_HIDE_FROM_ABI inline bool operator==(const memory_resource& __lhs, const memory_resource& __rhs) {
+  return &__lhs == &__rhs || __lhs.is_equal(__rhs);
+}
----------------



================
Comment at: libcxx/include/memory_resource:9
+
+// namespace std::pmr {
+//   class memory_resource {
----------------
Can you use `/* ... */` like the other synosis do. That makes it easier to copy paste code from the Standard.


================
Comment at: libcxx/include/memory_resource:26
+//   };
+// }
+
----------------
Please add the comparison operators too.


================
Comment at: libcxx/src/memory_resource.cpp:14
+namespace pmr {
+_LIBCPP_FUNC_VIS memory_resource::~memory_resource() = default;
+} // namespace pmr
----------------
What is the reason to put the destructor in the dylib?

When keeping it in the dylib it needs `_AVAILABLITY_MACROS`, grep for `_LIBCPP_AVAILABILITY_FORMAT` for an example.


================
Comment at: libcxx/test/std/mem/mem.res/memory_resource.pass.cpp:89
+    equal_resource r2{};
+    assert(r1 == r1);
+    assert(!r1.is_equal(r1));
----------------
Can you use the `comparision_tests.h` and validate the noexcept status and whether the function returns a `bool`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132796