This is an archive of the discontinued LLVM Phabricator instance.

Add a ReadWriteMemoryBuffer class that can map an input file for modification
ClosedPublic

Authored by zturner on Mar 7 2018, 3:07 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Mar 7 2018, 3:07 PM
zturner updated this revision to Diff 137494.Mar 7 2018, 3:09 PM

clang-format the patch.

rnk added a comment.Mar 7 2018, 5:23 PM

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.

labath added a comment.Mar 8 2018, 2:16 AM

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.

  • 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..

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.

zturner updated this revision to Diff 137626.Mar 8 2018, 11:44 AM

Added a test.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 8 2018, 12:37 PM
This revision was automatically updated to reflect the committed changes.