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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/trunk/include/llvm/Object/WindowsResource.h:184
 
-Error writeWindowsResourceCOFF(StringRef OutputFile, Machine MachineType,
+Error writeWindowsResourceCOFF(std::unique_ptr<MemoryBuffer> &OutputBuffer,
+                               llvm::COFF::MachineTypes MachineType,
----------------
Either make this a reference or a pointer, but not a reference to a `unique_ptr`.  Probably a reference is fine since it doesn't make sense to write something without a valid output buffer.


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:310-312
+  WindowsResourceCOFFWriter(std::unique_ptr<MemoryBuffer> &OutputBuffer,
+                            COFF::MachineTypes MachineType,
                             const WindowsResourceParser &Parser, Error &E);
----------------
Same thing here.


================
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));
 }
----------------
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.  


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:421
 Error WindowsResourceCOFFWriter::write() {
-  BufferStart = Buffer->getBufferStart();
+  BufferStart = const_cast<char *>(OutputBuffer->getBufferStart());
 
----------------
The previous code didn't need a `const_cast`.  Why does the new code?


Repository:
  rL LLVM

https://reviews.llvm.org/D34265