<div dir="ltr">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?</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 15, 2017 at 2:33 AM Pavel Labath <<a href="mailto:labath@google.com">labath@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 14 December 2017 at 21:16, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> Looking at it - MutableArrayRef just derives from ArrayRef.<br>
><br>
> I'm not sure why a similar design wouldn't be used here - with<br>
> WritableMemoryBuffer deriving from MemoryBuffer. Is there a good summary of<br>
> why the difference in design exists here?<br>
><br>
><br>
<br>
For me, the main reason that the MutableArrayRef/ArrayRef relation<br>
cannot be directly applied here is the fact that MemoryBuffer already<br>
has it's own class hierarchy. You have class MemoryBuffer and then (in<br>
the cpp file) you have MemoryBufferMem and MemoryBufferMMapFile, which<br>
both inherit from it. That means you have no nice way to insert<br>
WritableMemoryBuffer into this hierarchy -- you'd have to have two<br>
versions (either by spelling them out, or through some template<br>
trickery) of say MemoryBufferMem, one which inherits from<br>
WritableMemoryBuffer, and one which inherits straight from<br>
MemoryBuffer. (Incidentaly, these parallel heirarchies is exactly what<br>
we would get for free if we went for MemoryBuffer/const MemoryBuffer<br>
solution).<br>
<br>
If we want to make that happen (WritableMemoryBuffer "is-a"<br>
MemoryBuffer), then the only way forward I see is to pimpl-ify the<br>
MemoryBuffer class. So we would have something like:<br>
<br>
class MemoryBufferImpl {<br>
  static std::unique_ptr<MemoryBufferImpl> create(..., bool writable);<br>
};<br>
class MemoryBufferMmap: public MemoryBufferImpl { ... };<br>
class MemoryBufferMem: public MemoryBufferImpl { ... };<br>
<br>
class MemoryBuffer {<br>
  std::unique_ptr<MemoryBufferImpl> Impl;<br>
  static std::unique_ptr<MemoryBuffer> create(...) { return<br>
make_unique<MemoryBuffer>(MemoryBufferImpl::create(..., /*writable*/<br>
false); }<br>
};<br>
<br>
class WritableMemoryBuffer: public MemoryBuffer {<br>
  static std::unique_ptr<WritableMemoryBuffer> create(...) { return<br>
make_unique<MemoryBuffer>(MemoryBufferImpl::create(..., /*writable*/<br>
true); }<br>
};<br>
<br>
This will create two layers of unique_ptr, which is not ideal, but<br>
this can only be transitional stage. Now that there is nothing<br>
polymorphic about the MemoryBuffer class, I could create a follow-up<br>
patch which makes MemoryBuffer a move-only type and update all factory<br>
functions to return MemoryBuffer (or WritableMemoryBuffer) directly.<br>
(Or, the other way around might be even better -- first pimpl-ify<br>
MemoryBuffer, and then add WritableMemoryBuffer which inherits from<br>
it.)<br>
<br>
This is more intrusive than the current version of this patch, but I<br>
do think it makes things cleaner. Let me know if you want me to move<br>
in this direction.<br>
</blockquote></div>