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

Eric Beckmann via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 15:50:16 PDT 2017


ecbeckmann added inline comments.


================
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());
----------------
ruiu wrote:
> Why do you want to strip all newlines?
This mimics the behavior of rc.exe, which strips all the newlines from a manifest before putting it in the .res file.  However, spaces are left intact.


================
Comment at: lld/COFF/DriverUtils.cpp:430
+
+  char *Current = const_cast<char *>(Buf->getBufferStart());
+  writeResFileHeader(Current);
----------------
ruiu wrote:
> Rename Current Buf and use uint8_t instead of char for consistency
Hmmm there is already a variable named "Buf".


================
Comment at: lld/COFF/DriverUtils.cpp:430
+
+  char *Current = const_cast<char *>(Buf->getBufferStart());
+  writeResFileHeader(Current);
----------------
ecbeckmann wrote:
> ruiu wrote:
> > Rename Current Buf and use uint8_t instead of char for consistency
> Hmmm there is already a variable named "Buf".
Also const_char to type uint8_t* is not possible, and requires an additional cast.


================
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;
+};
----------------
ruiu wrote:
> You are not really using this struct. If you need only the size of the struct, define it as a constant.
I do use in in DriverUtils.cpp for a reinterpret_cast.  It helps I think for writing the resource header.


https://reviews.llvm.org/D34525





More information about the llvm-commits mailing list