[PATCH] D40291: [Support] Add WritableMemoryBuffer class

[David Blaikie via llvm-commits]

Seems pretty awkward to change the nature of the way WritableMemoryBuffer
is done - such that it's not a MemoryBuffer at all, because of these static
factory functions.

I think this could be addressed by making them private in the derived class:

  private:
    using base::createFunction1;
    using base::createFunction2;
etc.

Then you can't accidentally use WritableMemoryBuffer::createFunction1 and
be confused by why you got a non-WritableMemoryBuffer as a result.

& WritableMemoryBuffer can have whatever factory functions are appropriate
for itself.

On Thu, Dec 14, 2017 at 1:45 PM Zachary Turner <zturner at google.com> wrote:

> All of the methods for constructing a MemoryBuffer are static methods of
> the MemoryBuffer base class.  And they all return MemoryBuffers.  It
> doesn't seem like a good interface to have:
>
> struct MemoryBuffer {
>    static MemoryBuffer createNonWritableBuffer1();
>    static MemoryBuffer createNonWritableBuffer2();
> };
>
> struct WritableMemoryBuffer : public MemoryBuffer {
>   static WritableMemoryBuffer createWritableBuffer1();
>   static WritableMemoryBuffer createWritableBuffer2();
> };
>
> You can use private inheritance so someone can't say
> WritableMemoryBuffer::createNonWritableBuffer1() (since that operation
> doesn't really make sense), but now you lose the primary advantages of
> using inheritance in the first place, which is that you can cast to a base
> class pointer.
>
> There might be a case to be made for extracting the static factory
> interface out of the MemoryBuffer class and into some global functions, but
> that's a rather intrusive change.
>
> On Thu, Dec 14, 2017 at 1:28 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> Not sure I follow - a derived class wouldn't inherit/necessarily have any
>> relationship to its base class in the ways it's constructed, only in the
>> ways it's used, right?
>>
>> & if WritableMemoryBuffer is in any sense a MemoryBuffer (either by
>> conversion or otherwise) it seems WritableMemoryBuffer does provide all the
>> same operations (once constructed) as MemoryBuffer plus whatever additional
>> mutation operations it provides.
>>
>>
>> On Thu, Dec 14, 2017 at 1:22 PM Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> Creating a MutableMemoryBuffer requires totally different code than
>>> creating a non-mutable memory buffer.  They can fail in different ways, for
>>> example, and some ways of initializing a memory buffer only apply in one
>>> case or the other.
>>>
>>> This works for ArrayRef and MutableArrayRef because the only difference
>>> between the underlying type is its constness, but that doesn't work when
>>> the underlying type is a class that has a different interface when it's
>>> const vs non-const.
>>>
>>> On Thu, Dec 14, 2017 at 1:16 PM 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?
>>>>
>>>>
>>>> On Mon, Dec 4, 2017 at 2:32 PM David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>> I advocated for const-ifying ArrayRef when MutableArrayRef was added
>>>>> (so const all the existing ArrayRef, and then have non-const ArrayRef for
>>>>> when mutation is desired - avoiding the need to introduce an explicit
>>>>> MutableArrayRef type) but Chris wasn't a fan of that, so we have ArrayRef +
>>>>> MutableArrayRef (the argument being that most ArrayRefs are const, so
>>>>> making the more common case more verbose was unfavorable)
>>>>>
>>>>> I believe in that case MutableArrayRef has an implicit conversion
>>>>> operator to ArrayRef (or ArrayRef has a conversion from MutableArrayRef, I
>>>>> forget which) & expect we could do the same thing here, to be consistent?
>>>>>
>>>>> - Dave
>>>>>
>>>>> On Mon, Dec 4, 2017 at 9:00 AM Pavel Labath via Phabricator <
>>>>> reviews at reviews.llvm.org> wrote:
>>>>>
>>>>>> labath added a comment.
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> could you look at this by any chance?
>>>>>>
>>>>>> thanks.
>>>>>>
>>>>>>
>>>>>> https://reviews.llvm.org/D40291
>>>>>>
>>>>>>
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171214/60879421/attachment.html>