[PATCH] D47111: <experimental/memory_resource>: Implement monotonic_buffer_resource.

[strager via Phabricator via cfe-commits]

strager added inline comments.


================
Comment at: include/experimental/memory_resource:427
+    static _LIBCPP_CONSTEXPR const size_t __default_buffer_capacity = 1024;
+    static _LIBCPP_CONSTEXPR const size_t __default_buffer_alignment = 16;
+
----------------
Nit: Why isn't `__default_buffer_alignment` set to `alignof(max_align_t)` (which I think is 16)? I think `alignof(max_align_t)` is a good, portable default.


================
Comment at: include/experimental/memory_resource:431-432
+        __chunk_header *__next_;
+        char *__start_;
+        char *__cur_;
+        size_t __align_;
----------------
Nit: Would it make sense to use `std::byte` instead of `char`?


================
Comment at: test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp:32
+        assert(globalMemCounter.checkOutstandingNewEq(0));
+    }
+
----------------
Nit: This test is named allocate_in_geometric_progression. I don't see any multiplication or anything happening in this test case. Did you mean to call this test something else, like release_deletes_upstream_memory?


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_deallocate.pass.cpp:28
+        std::experimental::pmr::monotonic_buffer_resource mono1(std::experimental::pmr::new_delete_resource());
+        std::experimental::pmr::memory_resource & r1 = mono1;
+
----------------
Nit: What is the purpose of the `r1` variable? Why not use `mono1` everywhere instead (and rename `mono1` to `r1` if you like `r1` better than `mono1` as a name)?


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp:48-49
+    void *res = r1.allocate(64, 16);
+    assert(res == buffer);
+    assert(globalMemCounter.checkNewCalledEq(0));
+
----------------
Nit: These assertions look unrelated to exception safety to me. (The test is named allocate_exception_safety.) I'd delete these assertions.


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp:52-54
+    assert(res != buffer);
+    assert(globalMemCounter.checkNewCalledEq(1));
+    assert(globalMemCounter.checkDeleteCalledEq(0));
----------------
(Same comment as on lines 48-49 above. I'd delete these assertions.)


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_exception_safety.pass.cpp:67-68
+
+    upstream.which = std::experimental::pmr::new_delete_resource();
+    res = r1.allocate(last_new_size, 16);
+    assert(res != buffer);
----------------
Question about the intended behavior of `monotonic_buffer_resource`:

If line 68 instead allocated 1 byte, and the byte could fit in the original heap allocation (line 51), should `monotonic_buffer_resource` return a byte from that original resource? In other words, should the following test pass?

```
repointable_resource upstream(std::experimental::pmr::new_delete_resource());
std::experimental::pmr::monotonic_buffer_resource mono1(&upstream);

globalMemCounter.reset();
mono1.allocate(1, 1);
const size_t first_new_size = globalMemCounter.last_new_size;
bool mono1_has_spare_capacity = first_new_size == 1;

upstream.which = std::experimental::pmr::null_memory_resource();
try {
    mono1.allocate(first_new_size, 1); // Force allocation from upstream.
    assert(false);
} catch (const std::bad_alloc&) {
    // we expect this
}

globalMemCounter.reset();
mono1.allocate(1, 1);
if (mono1_has_spare_capacity) {
  assert(globalMemCounter.checkNewCalledEq(0));
} else {
  assert(globalMemCounter.checkNewCalledEq(1));
}
```

Similarly, if `monotonic_buffer_resource` was given a buffer, and that buffer still has spare capacity, should that capacity be reused? In other words, should the following test pass?

```
int first_allocation_size = 20;
int second_allocation_size = 30;

auto *upstream = std::experimental::pmr::null_memory_resource();
char buffer[64];
assert(sizeof buffer >= first_allocation_size + second_allocation_size);
std::experimental::pmr::monotonic_buffer_resource mono1(buffer, sizeof buffer, upstream);

globalMemCounter.reset();
void *first_allocation = mono1.allocate(first_allocation_size, 1);
assert(first_allocation == &buffer[0]);

try {
    mono1.allocate(sizeof buffer, 1); // Force allocation from upstream.
    assert(false);
} catch (const std::bad_alloc&) {
    // we expect this
}

void *second_allocation = mono1.allocate(second_allocation_size, 1);
assert(second_allocation == &buffer[second_allocation_size]);
```

I think these scenarios would be useful to test, in addition to the scenario in your test here.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47111