[PATCH] D34265: Switch external cvtres.exe for llvm's own resource library.In this patch, I flip the switch in DriverUtils from using the externalcvtres.exe tool to using the Windows Resource library in llvm.I also fixed a bug where .rsrc sections were...

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:356
 
-  ErrorOr<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-      FileOutputBuffer::create(OutputFile, FileSize);
-  if (!BufferOrErr) {
-    E = errorCodeToError(BufferOrErr.getError());
-    return;
-  }
-
-  Buffer = std::move(*BufferOrErr);
+  OutputBuffer = std::move(MemoryBuffer::getNewMemBuffer(FileSize));
 }
----------------
zturner wrote:
> ecbeckmann wrote:
> > zturner wrote:
> > > This is a bit strange.  First we initialize the member variable `OutputBuffer` with the constructor argument (in the initializer list), and then we overwrite....  one of the two items with a new memory buffer?  I'm not sure which one takes precedence here, but I think it's probably the constructor arg which means you're treating the constructor arg as an in/out parameter, which is pretty non-standard.
> > > 
> > > In any case, can you re-think this logic to make it more sensible?  Do you intend for this class to take ownership of the input parameter?  if so, then pass the `unique_ptr` by value, not by reference.  If not, then pass the `MemoryBuffer` by reference, and we don't need the `unique_ptr` at all.  
> > The idea is that we get an uninitialized reference to a MemBuf from the user, and then initialize it with the final COFF.  It must be uninitialized because in order to initialize a MemBuf you must know the size you have to allocate, and this should be calculated only within the WindowsResourceCOFFWriter class.  For all intents and purposes this parameter is an output, and is not used as an input in any way.
> > 
> > Therefore passing by reference won't work because we are the ones to initialize the buffer, it can't be done outside.  We also cannot pass unique_ptr by value because this parameter needs to be an output accessible to the user, we aren't using it as an input in any way.
> > 
> > Probably the best solution then is to take a MemoryBuffer pointer as input?
> In that case instead of returning an `Error`, how about returning an `Expected<unique_ptr<MemoryBuffer>>` from `writeWindowsCOFFResource`?
That seems like the best option


Repository:
  rL LLVM

https://reviews.llvm.org/D34265