[PATCH] D22206: COFF: drop the dependency on LIB.EXE for implibs

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits.


================
Comment at: COFF/Librarian.cpp:65-66
@@ +64,4 @@
+                             ArrayRef<const std::string> Strings) {
+  // The COFF string table consists of a 4-byte value which is the size of the
+  // table, including the length field itself.  This value is followed by the
+  // string content itself, which is an array of null-terminated C-style
----------------
Ugh, this seems too generic to me - I really prefer `size_t` because an architecture where `sizeof(std::vector<uint8_t>::size_type) != sizeof(size_t)` seems really exotic.

================
Comment at: COFF/Librarian.cpp:80
@@ +79,3 @@
+    B.resize(Pos + S.length() + 1);
+    strcpy(reinterpret_cast<char *>(&B[Pos]), S.c_str());
+    Pos += S.length() + 1;
----------------
`memcpy(reinterpret_cast<char *>(&B[Pos]), S.data(), S.length())` would be better because it's cheaper if the string is not NUL-terminated.

================
Comment at: COFF/Librarian.cpp:154
@@ +153,3 @@
+  NewArchiveMember createShortImport(StringRef Sym, uint16_t Ordinal,
+                                     ImportNameType NameType, bool isData); };
+
----------------
Move `}` to the next line.

================
Comment at: COFF/Librarian.cpp:155
@@ +154,3 @@
+                                     ImportNameType NameType, bool isData); };
+
+NewArchiveMember
----------------
Add `}` to close the anonymous namespace.


http://reviews.llvm.org/D22206