[PATCH] D40291: [Support] Add WritableMemoryBuffer class

[David Blaikie via llvm-commits]

What if the two existing derived classes were templated on their base
class? (so you could have MemoryBufferMem<WritableMemoryBuffer> and
MemoryBufferMem<MemoryBuffer>) would that be reasonable?

On Fri, Dec 15, 2017 at 2:33 AM Pavel Labath <labath at google.com> wrote:

> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171215/b25fa4e3/attachment.html>