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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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)
----------------
zturner wrote:
> 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.
How can you catch memory exhaustion error if you use `new`?


https://reviews.llvm.org/D39449