We currently have two MemoryBuffer abstractions. The first is the most commonly used, and maps a file for read only access. The second is called WritableMemoryBuffer and maps a file privately (e.g. using copy-on-write) in case you need to make local modifications. But until this patch, we have no abstraction that can map a file in a way that allows to commit modifications back to the underlying file. There are no users of this yet (in this patch anyway), but I have a patch locally which will use this in a followup.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
We should have a unit test where we fill a file with some text and then modify it in place, like toUpper every character or something. Otherwise, looks good.
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..
llvm/include/llvm/Support/MemoryBuffer.h | ||
---|---|---|
216 ↗ | (On Diff #137494) | 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. |
llvm/lib/Support/MemoryBuffer.cpp | ||
372 ↗ | (On Diff #137494) | I think this is not used. |
It might be clearer, to someone reading the code, but probably less obvious to someone writing new code. People familiar with a posix-like api expect the current F_Append semantics, so I think we should make those assumptions continue to hold.
I'll work on a test.