Quuxplusone marked 11 inline comments as done.
Quuxplusone 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;
+
----------------
strager wrote:
> 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.
Will users be more upset if `__default_buffer_alignment` is 16-instead-of-`???`
on exotic platform `???`; or more upset if `__default_buffer_alignment` is
//usually// 16 but then sometimes is `???`-instead-of-16 on exotic platforms? I
tend to think that "usually 16, sometimes something else" is worse behavior
software-engineering-wise than "always 16, even when something else might
arguably be better on exotic platform `???`."
================
Comment at: include/experimental/memory_resource:431-432
+ __chunk_header *__next_;
+ char *__start_;
+ char *__cur_;
+ size_t __align_;
----------------
strager wrote:
> Nit: Would it make sense to use `std::byte` instead of `char`?
IMHO "strong no"; `char` is the standard core-language type that takes up one
byte. `std::byte` is an enum type that we'd have to drag in from a header
(cost), and otherwise behaves just like `char` (no benefit). But it's certainly
a non-functional nit and I would not put up a fight if libc++ wanted it that
way. :)
================
Comment at: include/experimental/memory_resource:433
+ char *__cur_;
+ size_t __align_;
+ size_t __allocation_size() {
----------------
strager wrote:
> > Eric suggests replacing size_t __align_ with uint8_t __log2_align_. I'm
> > amenable to this idea, but I'd want to know what's the best way to compute
> > the log2 of a user-supplied number.
>
> Perhaps `std::log2p1` could help:
> https://en.cppreference.com/w/cpp/numeric/log2p1
Yes, but that's C++2a; I imagine some fiddliness would be needed to use it in
C++17 library code (which in turn is actually compiled as C++11 or '14, I
forget).
================
Comment at:
test/libcxx/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_in_geometric_progression.pass.cpp:32
+ assert(globalMemCounter.checkOutstandingNewEq(0));
+ }
+
----------------
strager wrote:
> 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?
Prior to the resolution of [LWG
3120](https://cplusplus.github.io/LWG/issue3120), this test had tested
something else. I guess now it //should// be more like
"release_resets_initial_buffer.pass.cpp".
================
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;
+
----------------
strager wrote:
> 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)?
`r1` has a different static type. For example, here it's testing that
`r1.allocate(50)` actually makes a virtual call to
`monotonic_buffer_resource::do_allocate`. But I guess it doesn't really matter;
I could use `mono1` for everything just as well.
================
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));
+
----------------
strager wrote:
> Nit: These assertions look unrelated to exception safety to me. (The test is
> named allocate_exception_safety.) I'd delete these assertions.
Depends on your test-overfitting philosophy. These assertions are still
expected to pass 100% of the time — if they fail, then something is definitely
wrong. And they're cheap to execute. So IMHO they might as well be there, as
"self-documenting code."
================
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);
----------------
strager wrote:
> 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.
> If [the request] could fit in the original heap allocation, should
> `monotonic_buffer_resource` return a byte from that original allocation?
> Similarly, if `monotonic_buffer_resource` was given a buffer, and that buffer
> still has spare capacity, should that capacity be reused?
In both cases, the Standard says "absolutely not":
[mem.res.monotonic.buffer.mem#5](http://eel.is/c++draft/mem.res.monotonic.buffer.mem#5).
I can add these tests.
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
