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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
