[PATCH] D40291: [Support] Add WritableMemoryBuffer class

[Pavel Labath via llvm-commits]

On 22 November 2017 at 17:30, Rafael Avila de Espindola
<rafael.espindola at gmail.com> wrote:
> Pavel Labath via Phabricator <reviews at reviews.llvm.org> writes:
>> +
>> +TEST_F(MemoryBufferTest, writableSlice) {
>> +  // Create a file initialized with some data
>> +  int FD;
>> +  SmallString<64> TestPath;
>> +  sys::fs::createTemporaryFile("MemoryBufferTest_WritableSlice", "temp", FD,
>> +                               TestPath);
>> +  FileRemover Cleanup(TestPath);
>> +  raw_fd_ostream OF(FD, true, /*unbuffered=*/true);
>
> Why do you need it to be unbuffered?

I don't. I inherited that part from Zach :)

>
>> +/// This interface is similar to MemoryBuffer, but allows writing to the
>> +/// underlying contents.  It only supports creation methods that are guaranteed
>> +/// to produce a writable buffer.  For example, mapping a file read-only is not
>> +/// supported.
>> +class WritableMemoryBuffer {
>> +  std::unique_ptr<MemoryBuffer> Buffer;
>> +
>
> This part of the patch is a bit odd, but I can't immediately think of
> anything better that keeps MemoryBuffer and WritableMemoryBuffer as
> distinct types.
>
> We could make MemoryBuffer non virtual with a PIMPL. That would make
> MemoryBuffer and WritableMemoryBuffer symmetrical simple wrappers, but it
> is not clear if it is worth it.

Yeah, it's a bit strange.. The pimpl would introduce an extra layer of
indirection to MemoryBuffer, which does not sound like a good idea, as
we probably care more about it's performance than of this new class.

IMHO, the cleanest solution here would be to actually not make these
distinct types, and use *const* MemoryBuffer to denote an immutable
piece of memory. I did not go with that because this is a "central
piece of infrastructure" :) and it would require fixing up all current
uses of MemoryBuffer (well.. almost all. I did find a couple of places
where we actually are const_casting the buffer contents and writing to
it).


>
> So this LGTM but please get a second LGTM as this is a pretty central
> piece of infrastructure.
Do you have a suggestion who would that person be? (Chandler seems to
be listed as the owner of Support, but I don't see him reviewing many
patches there...)

thank you,
pl