[PATCH] D40291: [Support] Add WritableMemoryBuffer class

[Rafael Avila de Espindola via llvm-commits]

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?

> +/// 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.

So this LGTM but please get a second LGTM as this is a pretty central
piece of infrastructure.

Cheers,
Rafael