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

[Saleem Abdulrasool via llvm-commits]

compnerd marked 7 inline comments as done.

================
Comment at: COFF/Librarian.cpp:32
@@ +31,3 @@
+static bool is32bit() {
+  switch (lld::coff::Config->Machine) {
+  default:
----------------
ruiu wrote:
> Why don't you add `using namespace lld::coff` at beginning of this file?
No strong reason, I just tend to prefer explicit namespace, fine by me to use that though :-).

================
Comment at: COFF/Librarian.cpp:64-65
@@ +63,4 @@
+                             ArrayRef<const std::string> Strings) {
+  auto Pos = B.size();
+  auto Offset = B.size();
+
----------------
ruiu wrote:
> I prefer `size_t` over `auto`.
If we are using the type, Id rather use `std::vector<uint8_t>::size_type`.  Changed to that.

================
Comment at: COFF/Librarian.cpp:104
@@ +103,3 @@
+namespace {
+class ObjectFactory {
+  using u16 = support::ulittle16_t;
----------------
ruiu wrote:
> 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.
This conflicts with the other comment to move the functions out of the class.  Once we decide between the two, Ill update the patch.

================
Comment at: COFF/Librarian.cpp:120
@@ +119,3 @@
+
+  NewArchiveMember createImportDescriptor(std::vector<uint8_t> &Buffer) {
+    static const uint32_t NumberOfSections = 2;
----------------
ruiu wrote:
> Can you define this and other member functions as out-of-line functions?
We could, it was meant as a means to avoid re-creating the strings for the ImportDescriptor and NullThunk symbol name.  This isn't the hot path, so we can certainly do that.  I don't have too strong of an opinion.


http://reviews.llvm.org/D22206