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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

As a convention, please write a short summary (ideally less than 80 chars) on the first line of a commit message, so that they are not wrapped around
... like this.



================
Comment at: lld/COFF/DriverUtils.cpp:599
   for (MemoryBufferRef MB : MBs) {
-    // We store the temporary file in a vector to avoid deletion
-    // before running cvtres
-    ResFiles.emplace_back("resource-file", "res");
-    TemporaryFile& ResFile = ResFiles.back();
-    // Write the content of the resource in a temporary file
-    std::error_code EC;
-    raw_fd_ostream OS(ResFile.Path, EC, sys::fs::F_None);
-    if (EC)
-      fatal(EC, "failed to open " + ResFile.Path);
-    OS << MB.getBuffer();
-    OS.close();
-
-    E.add(ResFile.Path);
+    auto Binary = check(object::createBinary(MB));
+    object::WindowsResource *RF =
----------------
In LLD we avoid `auto` unless a concrete type is obvious, so replace `auto` with a concrete type.


================
Comment at: lld/COFF/DriverUtils.cpp:603
+    if (!RF)
+      fatal("Cannot compile non-resource file as resource");
+    if (auto EC = Parser.parse(RF))
----------------
Error messages should start with a lowercase letter.


================
Comment at: lld/COFF/DriverUtils.cpp:605
+    if (auto EC = Parser.parse(RF))
+      fatal(EC, "Failed to parse .res file.");
   }
----------------
And shouldn't end with a full stop. By following that rule, you can concatenate multiple error messages like this.

  error while reading file: failed to parse .res file: unexpected EOF


================
Comment at: lld/COFF/DriverUtils.cpp:608
 
-  E.run();
-  return File.getMemoryBuffer();
+  std::unique_ptr<MemoryBuffer> OutputBuffer;
+  if (auto EC = llvm::object::writeWindowsResourceCOFF(OutputBuffer,
----------------
In LLD we usually use much shorter name. This would be just `MB`.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:209
+            FileBuffer->getBufferStart());
+  ;
+  error(FileBuffer->commit());
----------------
Stray `;`.


https://reviews.llvm.org/D34265