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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: include/experimental/memory_resource:429
+    size_t __capacity_;
+    size_t __alignment_;
+    size_t __used_;
----------------
EricWF wrote:
> I can't imagine we'll need more than 1 byte to represent the alignment.
Even assuming for the sake of argument that that's right, it wouldn't buy us anything here because of padding, would it?

At the moment, `__alignment_` needs to have enough range to store the `align` parameter passed to `monotonic_buffer_resource::do_allocate`. In an SSE world we should not blithely assume that `align < 256`. We //could// store `lg(align)` in a single byte since `align` is always a power of two and less than 2^64 — but then we're back to the first argument, which is that it'll be padded to 8 bytes anyway so what do we gain.


================
Comment at: include/experimental/memory_resource:474
+protected:
+    void* do_allocate(size_t __bytes, size_t __alignment);
+
----------------
EricWF wrote:
> Lets add `override` to these functions.
I grepped for "override" in `include/` and saw exactly one (accidental?) use in `experimental/filesystem`, so I thought probably libc++ had a policy not to use it for portability reasons. I'll add it throughout, but would like someone to explicitly confirm that

(A) it's okay to use in this header and wouldn't need to be guarded with a `_LIBCPP_OVERRIDE` macro or anything

(B) Arthur should //not// bother trying to add it to any //other// headers, not even "for consistency"


================
Comment at: src/experimental/memory_resource.cpp:217
+{
+    if (void *result = try_allocate_from_chunk(&__original_, bytes, align)) {
+        return result;
----------------
EricWF wrote:
> Drop the braces for conditionals and loops with single statements.
*shiver* but okay.


================
Comment at: src/experimental/memory_resource.cpp:237
+
+    void *result = __res_->allocate(aligned_capacity, align);
+    __monotonic_buffer_header *header = (__monotonic_buffer_header *)((char *)result + aligned_capacity - header_size);
----------------
For reference, here is where we ask the upstream for a block aligned according to the user's `align`.
It occurs to me that the upstream might not be able to satisfy such a request (actually, `new_delete_resource` works that way for me because libc++ doesn't support aligned new and delete on OSX), which would make the upstream throw `bad_alloc`. We handle this by passing along the exception. We //could// conceivably handle it by retrying with
```
    aligned_capacity += align;
    __res_->allocate(aligned_capacity, alignof(max_align_t));
    header->__alignment_ = alignof(max_align_t);
```
but I'm not sure that that's any of `monotonic_buffer_resource`'s business. Happy to make the patch if you think it ought to be.


Repository:
  rCXX libc++

https://reviews.llvm.org/D47111