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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D47111#1114284, @Quuxplusone wrote:

> Refactor to remove unused fields from the header structs.
>
> Before this change, `sizeof(monotonic_buffer_resource) == 56` and the overhead per allocated chunk was `40` bytes.
>  After this change, `sizeof(monotonic_buffer_resource) == 48` and the overhead per allocated chunk is `24` bytes.


Woo! This is looking great!

I'm done looking at the implementation, I'll take a pass at the tests tomorrow. Some pointers in general:

1. Tests should be named after the component they test, not how they're testing it.
2. All the tests for a single component should be in the same file.
3. You can use `LIBCPP_ASSERT` to check specific implementation details, like exactly how much our allocation sizes grow.

(1) and (2) are important for maintaining the tests. (3) will probably be useful for actually writing meaningful tests despite
most observable behavior being implementation specific.



================
Comment at: include/experimental/memory_resource:425
+
+struct __monotonic_buffer_chunk_header;
+
----------------
Can these classes be members of `monotonic_buffer_resource`?


================
Comment at: include/experimental/memory_resource:451
+
+    monotonic_buffer_resource(void* __buffer, size_t __buffer_size, memory_resource* __upstream);
+
----------------
Lets keep these constructors inline.


================
Comment at: include/experimental/memory_resource:467
+
+    ~monotonic_buffer_resource() override; // key function
+
----------------
Need _LIBCPP_FUNC_VIS


================
Comment at: include/experimental/memory_resource:471
+
+    void release();
+
----------------
I think it would be nice for `release` to be inline. What do you think?


================
Comment at: include/experimental/memory_resource:489
+private:
+    size_t __previous_allocation_size() const;
+
----------------
`__previous_allocation_size` isn't used in the header, so it shouldn't be declared there. We don't want to expose more symbols than we need to.


================
Comment at: src/experimental/memory_resource.cpp:212
+
+monotonic_buffer_resource::monotonic_buffer_resource(void* buffer, size_t buffer_size, memory_resource* upstream)
+    : __res_(upstream)
----------------
In general, please wrap to 80 characters.


================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/nothing_to_do.pass.cpp:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
We don't actually need this file, though older libc++ tests will often include it.

It's only needed to keep empty directories around, but this one has children.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47111