[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
llvm-commits at lists.llvm.org
Fri Jun 16 15:39:48 PDT 2017
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
More information about the llvm-commits
mailing list