[PATCH] D40291: [Support] Add WritableMemoryBuffer class

[Pavel Labath via llvm-commits]

On 14 December 2017 at 21:16, David Blaikie <dblaikie at gmail.com> wrote:
> Looking at it - MutableArrayRef just derives from ArrayRef.
>
> I'm not sure why a similar design wouldn't be used here - with
> WritableMemoryBuffer deriving from MemoryBuffer. Is there a good summary of
> why the difference in design exists here?
>
>

For me, the main reason that the MutableArrayRef/ArrayRef relation
cannot be directly applied here is the fact that MemoryBuffer already
has it's own class hierarchy. You have class MemoryBuffer and then (in
the cpp file) you have MemoryBufferMem and MemoryBufferMMapFile, which
both inherit from it. That means you have no nice way to insert
WritableMemoryBuffer into this hierarchy -- you'd have to have two
versions (either by spelling them out, or through some template
trickery) of say MemoryBufferMem, one which inherits from
WritableMemoryBuffer, and one which inherits straight from
MemoryBuffer. (Incidentaly, these parallel heirarchies is exactly what
we would get for free if we went for MemoryBuffer/const MemoryBuffer
solution).

If we want to make that happen (WritableMemoryBuffer "is-a"
MemoryBuffer), then the only way forward I see is to pimpl-ify the
MemoryBuffer class. So we would have something like:

class MemoryBufferImpl {
  static std::unique_ptr<MemoryBufferImpl> create(..., bool writable);
};
class MemoryBufferMmap: public MemoryBufferImpl { ... };
class MemoryBufferMem: public MemoryBufferImpl { ... };

class MemoryBuffer {
  std::unique_ptr<MemoryBufferImpl> Impl;
  static std::unique_ptr<MemoryBuffer> create(...) { return
make_unique<MemoryBuffer>(MemoryBufferImpl::create(..., /*writable*/
false); }
};

class WritableMemoryBuffer: public MemoryBuffer {
  static std::unique_ptr<WritableMemoryBuffer> create(...) { return
make_unique<MemoryBuffer>(MemoryBufferImpl::create(..., /*writable*/
true); }
};

This will create two layers of unique_ptr, which is not ideal, but
this can only be transitional stage. Now that there is nothing
polymorphic about the MemoryBuffer class, I could create a follow-up
patch which makes MemoryBuffer a move-only type and update all factory
functions to return MemoryBuffer (or WritableMemoryBuffer) directly.
(Or, the other way around might be even better -- first pimpl-ify
MemoryBuffer, and then add WritableMemoryBuffer which inherits from
it.)

This is more intrusive than the current version of this patch, but I
do think it makes things cleaner. Let me know if you want me to move
in this direction.