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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Sun Jul 10 17:24:20 PDT 2016

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.



More information about the llvm-commits mailing list