[PATCH] D44230: Add a ReadWriteMemoryBuffer class that can map an input file for modification

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

It seems fine, but a test would certainly be useful. Two things had come to mind, but I don't have a definitive answer to either of them:

- the difference between a WritableMemoryBuffer and ReadWriteMemoryBuffer is not very obvious. After all, both of them support reading and writing... Maybe name the second one something like WriteThroughMemoryBuffer, to emphasize that the changes are not local?
- Right now the F_NoTrunc flag is ignored if one specifies F_Append (we always treat it as present). However, the Append&&!NoTrunc combination also has a well defined (although not extremely useful) semantics. I wonder if it wouldn't be clearer to make the two completely independent. This would require adding F_NoTrunc to every existing call that specifies F_Append, but I currently count only about half a dozen of those..



================
Comment at: llvm/include/llvm/Support/MemoryBuffer.h:216
+/// This class is an extension of MemoryBuffer, which allows write access to
+/// the underlying contents and committing those changes to the original source.
+/// It only supports creation methods that are guaranteed to produce a writable
----------------
The word "commit" bring database associations, like the changes being atomic and not permanent until you explicitly apply them, but that's not true. I would just say the changes are written back to the original file.


================
Comment at: llvm/lib/Support/MemoryBuffer.cpp:372
+
+  static int PageSize = sys::Process::getPageSize();
+
----------------
I think this is not used.


https://reviews.llvm.org/D44230