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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: COFF/Librarian.cpp:32
@@ +31,3 @@
+static bool is32bit() {
+  switch (lld::coff::Config->Machine) {
+  default:
----------------
Why don't you add `using namespace lld::coff` at beginning of this file?

================
Comment at: COFF/Librarian.cpp:62
@@ +61,3 @@
+
+static void writeStringTable(std::vector<uint8_t> &B,
+                             ArrayRef<const std::string> Strings) {
----------------
Can you describe the structure of the string table briefly? It consists of the size field followed by null-terminated strings.

================
Comment at: COFF/Librarian.cpp:64-65
@@ +63,4 @@
+                             ArrayRef<const std::string> Strings) {
+  auto Pos = B.size();
+  auto Offset = B.size();
+
----------------
I prefer `size_t` over `auto`.

================
Comment at: COFF/Librarian.cpp:74
@@ +73,3 @@
+
+  support::ulittle32_t Length(B.size() - Offset);
+  memcpy(&B[Offset], &Length, sizeof(Length));
----------------
Add a comment that this is to backfill the size field.

================
Comment at: COFF/Librarian.cpp:104
@@ +103,3 @@
+namespace {
+class ObjectFactory {
+  using u16 = support::ulittle16_t;
----------------
I'd be nice to add comments for this class to say that most code in this class is to create an empty import table which has almost fixed contents and you don't need to understand the details unless you are particularly interested in the import table structure.

================
Comment at: COFF/Librarian.cpp:120
@@ +119,3 @@
+
+  NewArchiveMember createImportDescriptor(std::vector<uint8_t> &Buffer) {
+    static const uint32_t NumberOfSections = 2;
----------------
Can you define this and other member functions as out-of-line functions?

================
Comment at: COFF/Librarian.cpp:125
@@ +124,3 @@
+
+    /* COFF Header */
+    coff_file_header Header{
----------------
Please use C++-style comments (//) instead of C-style (/**/).

================
Comment at: COFF/Librarian.cpp:427
@@ +426,3 @@
+// import files to that file.
+void writeImportLibrary() {
+  std::vector<NewArchiveMember> Members;
----------------
You can define this as

  void lld::coff::writeImportLibrary()

to remove the namespace definitions surrounding this.


http://reviews.llvm.org/D22206