[PATCH] D19351: ELF: Add initial ThinLTO support.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:22
@@ -21,2 +21,3 @@
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
----------------
Ditto.

================
Comment at: ELF/Driver.h:17
@@ -16,2 +16,3 @@
 #include "llvm/ADT/StringRef.h"
+#include "llvm/IR/LLVMContext.h"
 #include "llvm/Option/ArgList.h"
----------------
Do you need this?

================
Comment at: ELF/Error.cpp:55
@@ -52,1 +54,3 @@
 
+void diagHandler(const llvm::DiagnosticInfo &DI) {
+  llvm::DiagnosticPrinterRawOStream DPOS(llvm::errs());
----------------
Even though this function is passed as std::function<> object, it should be verb rather than noun because it is a function.

================
Comment at: ELF/Error.h:16-18
@@ +15,5 @@
+namespace llvm {
+
+class DiagnosticInfo;
+
+}
----------------
Remove blank lines.

================
Comment at: ELF/LTO.cpp:307-311
@@ -223,1 +306,7 @@
+std::vector<std::unique_ptr<InputFile>> BitcodeCompiler::compile() {
+  auto Objs = compileLto();
+  auto ThinLtoObjs = compileThinLto();
+  for (auto &Obj : ThinLtoObjs)
+    Objs.push_back(std::move(Obj));
+  return Objs;
 }
----------------
I can see the reason why you chose to use `auto` here, but still I'd use real types for consistency even though it's a bit too verbose.

================
Comment at: ELF/LTO.h:33-35
@@ +32,5 @@
+namespace llvm {
+
+class Target;
+
+}
----------------
Remove blank lines.

================
Comment at: ELF/SymbolTable.cpp:58-62
@@ -56,1 +57,7 @@
 
+template <class ELFT>
+SymbolTable<ELFT>::SymbolTable() {}
+
+template <class ELFT>
+SymbolTable<ELFT>::~SymbolTable() {}
+
----------------
Is this what clang-format formatted?

================
Comment at: ELF/SymbolTable.cpp:242-245
@@ +241,6 @@
+static bool isRegularLtoInputFile(InputFile *File) {
+  auto *BC = dyn_cast<BitcodeFile>(File);
+  if (!BC)
+    return false;
+  return !BC->ThinLto;
+}
----------------
You can write

  if (auto *BC = dyn_cast<BitcodeFile>(File))
    return !BC->ThinLto;
  return false;


http://reviews.llvm.org/D19351