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

[Rui Ueyama via llvm-commits]

ruiu added a comment.

I think you want to create a Librarian.h for functions/classes in Librarian.cpp.


================
Comment at: COFF/Librarian.cpp:10
@@ +9,3 @@
+//
+// This file contains functions for the Librarian.
+//
----------------
You want to describe what Librarian is. (Readers already know that this file is for "Librarian" whatever it is because this is Librarian.cpp.)

================
Comment at: COFF/Librarian.cpp:29
@@ +28,3 @@
+namespace coff {
+namespace {
+static bool is32bit() {
----------------
It is recommended by the LLVM coding style that you minimize the region that an anonymous namespace encloses. Since static functions don't have to be in an anonymous namespace, you want to exclude them from this anonymous namespace.

================
Comment at: COFF/Librarian.cpp:54
@@ +53,3 @@
+}
+static char *writeFileHeader(char *P, uint32_t Sections, uint32_t RawDataSize,
+                             uint32_t Symbols) {
----------------
Add a newline before this line.

================
Comment at: COFF/Librarian.cpp:132
@@ +131,3 @@
+        ImportDescriptorSymbolName(("__IMPORT_DESCRIPTOR_" + Library).str()),
+        NullImportDescriptorSymbolName("__NULL_IMPORT_DESCRIPTOR"),
+        NullThunkSymbolName(("\x7f" + Library + "_NULL_THUNK_DATA").str()) {}
----------------
Since this is actually a constant, I'd make this a non-member constant string.

================
Comment at: COFF/Librarian.cpp:140
@@ +139,3 @@
+
+    uint32_t Size =
+        /* COFF Header */
----------------
It seems that you don't need to compute the size beforehand. Instead, you can construct a std::vector or something like that and then get the size of the constructed object at end of this function. (This would be a bit inefficient, but because this function is not hot at all, it should be fine.)

Or you can create a buffer that is large enough to store any import library file and truncate it at end of this funciton.


Repository:
  rL LLVM

http://reviews.llvm.org/D22206