[PATCH] D34525: Replace trivial use of external rc.exe by writing our own .res file.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I think a better way of creating manifest file contents is to construct a temporary std::vector and then copy it to a MemoryBuffer after you append everything to the vector. This way, you don't need to compute the size of the result ahead of time.



================
Comment at: lld/COFF/DriverUtils.cpp:380
+static std::unique_ptr<MemoryBuffer>
+getMemoryBufferForManifestRes(std::string &Manifest) {
+  size_t ResSize = object::WIN_RES_MAGIC_SIZE + object::WIN_RES_NULL_ENTRY_SIZE;
----------------
s/get/create/ as it creates a new instance of MemoryBuffer.


================
Comment at: lld/COFF/DriverUtils.cpp:381-386
+  size_t ResSize = object::WIN_RES_MAGIC_SIZE + object::WIN_RES_NULL_ENTRY_SIZE;
+  ResSize += sizeof(object::WinResHeaderPrefix);
+  ResSize += sizeof(object::WinResIDs);
+  ResSize += sizeof(object::WinResHeaderSuffix);
+  ResSize += Manifest.size();
+  ResSize = alignTo(ResSize, object::WIN_RES_DATA_ALIGNMENT);
----------------
It feels to me that this is cleaner.

  size_t Size = alignTo(object::WIN_RES_MAGIC_SIZE +
                        object::WIN_RES_NULL_ENTRY_SIZE +
                        sizeof(object::WinResHeaderPrefix) +
                        sizeof(object::WinResIDs) +
                        sizeof(object::WinResHeaderSuffix) +
                        Manifest.size(),
                        object::WIN_RES_DATA_ALIGNMENT);


================
Comment at: lld/COFF/DriverUtils.cpp:424-426
+  // Strip all newlines from the Manifest.
+  Manifest.erase(std::remove(Manifest.begin(), Manifest.end(), '\n'),
+                 Manifest.end());
----------------
Why do you want to strip all newlines?


================
Comment at: lld/COFF/DriverUtils.cpp:428
 
-  // Open the temporary file for writing.
-  std::error_code EC;
-  raw_fd_ostream Out(RCFile.Path, EC, sys::fs::F_Text);
-  if (EC)
-    fatal(EC, "failed to open " + RCFile.Path);
-
-  // Write resource script to the RC file.
-  Out << "#define LANG_ENGLISH 9\n"
-      << "#define SUBLANG_DEFAULT 1\n"
-      << "#define APP_MANIFEST " << Config->ManifestID << "\n"
-      << "#define RT_MANIFEST 24\n"
-      << "LANGUAGE LANG_ENGLISH, SUBLANG_DEFAULT\n"
-      << "APP_MANIFEST RT_MANIFEST {\n";
-  quoteAndPrint(Out, createManifestXml());
-  Out << "}\n";
-  Out.close();
-
-  // Create output resource file.
-  TemporaryFile ResFile("output-resource", "res");
-
-  Executor E("rc.exe");
-  E.add("/fo");
-  E.add(ResFile.Path);
-  E.add("/nologo");
-  E.add(RCFile.Path);
-  E.run();
-  return ResFile.getMemoryBuffer();
+  std::unique_ptr<MemoryBuffer> Buf = getMemoryBufferForManifestRes(Manifest);
+
----------------
Since you are not using the contents of `Manifest`, pass `Manifest.size()` instead of `Manifest`.


================
Comment at: lld/COFF/DriverUtils.cpp:430
+
+  char *Current = const_cast<char *>(Buf->getBufferStart());
+  writeResFileHeader(Current);
----------------
Rename Current Buf and use uint8_t instead of char for consistency


================
Comment at: llvm/include/llvm/BinaryFormat/COFF.h:50
+// The signature bytes that start a .res file.
+static const uint8_t WinResMagic[] = {0x00, 0x00, 0x00, 0x00, 0x20, 0x00,
+                                      0x00, 0x00, 0xff, 0xff, 0x00, 0x00,
----------------
Since all the other Magics are of `const char[]`, you want to use char.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:49-57
 
+const size_t WIN_RES_MAGIC_SIZE = 16;
+
+const size_t WIN_RES_NULL_ENTRY_SIZE = 16;
+
+const uint32_t WIN_RES_HEADER_ALIGNMENT = sizeof(uint32_t);
+
----------------
Remove blank lines. Replace sizeof(uint32_t) with 4 because it is always 4.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:67-72
+struct WinResIDs {
+  const uint16_t TypeFlag = 0xffff;
+  support::ulittle16_t TypeID;
+  const uint16_t NameFlag = 0xffff;
+  support::ulittle16_t NameID;
+};
----------------
You are not really using this struct. If you need only the size of the struct, define it as a constant.


https://reviews.llvm.org/D34525