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

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 15:56:09 PDT 2017


ruiu 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());
----------------
ecbeckmann wrote:
> 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.
If you don't do this and it still works, don't do that. We don't have to mimic all the details.


================
Comment at: lld/COFF/DriverUtils.cpp:430
+
+  char *Current = const_cast<char *>(Buf->getBufferStart());
+  writeResFileHeader(Current);
----------------
ecbeckmann wrote:
> 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.
Then please rename the conflicting one. `Buf` is used as a pointer to uint8_t in LLD very often, so not naming it is a bit confusing.


================
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;
+};
----------------
ecbeckmann wrote:
> 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.
Then maybe you want to remove the default value 0xffff. You are not using it.


https://reviews.llvm.org/D34525





More information about the llvm-commits mailing list