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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/experimental/memory_resource:428
+    __monotonic_buffer_header *__next_;
+    size_t __capacity_;
+    size_t __alignment_;
----------------
Can't we determine the capacity in most cases simply using `static_cast<char*>(this) - static_cast<char*>(__start_)`?


================
Comment at: include/experimental/memory_resource:429
+    size_t __capacity_;
+    size_t __alignment_;
+    size_t __used_;
----------------
Quuxplusone wrote:
> 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.
> Even assuming for the sake of argument that that's right, it wouldn't buy us anything here because of padding, would it?

It could. (A) we don't actually need to include the types trailing padding when tail allocating it as a part of a buffer.
and less importantly (B) I'm not sure all ABI's require the trailing padding of a type be included when that type is a data member of another type (I might just be wrong on this).

I'm also looking into other ways of improving the way your implementation packs data, which may cause this to be beneficial. 


================
Comment at: include/experimental/memory_resource:474
+protected:
+    void* do_allocate(size_t __bytes, size_t __alignment);
+
----------------
Quuxplusone wrote:
> 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"
The header already assumes a full C++11 implementation (it uses `std::tuple`), the `override` keyword will bi available. No need for a special macro.



================
Comment at: test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic_buffer.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Instead of testing the whole of `monotonic_buffer` in a single file, this test should be broken up into separate files. Roughly one per function, or when it makes sense, one per subsection for the lowest level of heading (For example a single test for all constructors under `memory.buffer.ctor` ).


Repository:
  rCXX libc++

https://reviews.llvm.org/D47111