[PATCH] D39449: Rewrite FileOutputBuffer as two separate classes.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:84-85
+    std::error_code EC;
+    MemoryBlock MB = Memory::allocateMappedMemory(
+        Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
+    if (EC)
----------------
ruiu wrote:
> zturner wrote:
> > Why can't we just use `new` here?
> I didn't design `Memory` class, but it looks like that class can be instantiated only via allocateMappedMemory.
Right, but why do we even need to use the `Memory` class at all?  It seems like overkill.  The comments say:

```
    /// This method allocates a block of memory that is suitable for loading
    /// dynamically generated code (e.g. JIT). 
```

Since we don't care about the protection (RWX) of the temporary storage, and only about the protection of the final file on disk, it seems like this is unnecessary.

Instead of storing `OwningMemoryBlock` as a member of the class, you could just store `std::vector<uint8_t>`.

There are fewer failure paths this way, as it's literally just one call to `new`, whereas `allocateMappedMemory` has several ways it can return errors.


================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:62
 
-  if (EC)
+  ~OnDiskBuffer() {
+    // Close the mapping before deleting the temp file, so that the removal
----------------
Maybe want to add `override` to this and the other class's destructor


https://reviews.llvm.org/D39449