[PATCH] D29892: ar: add llvm-dlltool support

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: include/llvm/DlltoolDriver/DlltoolDriver.h:25
+#endif
\ No newline at end of file

----------------
Your file doesn't end with '\n'. Please fix.


================
Comment at: include/llvm/Object/COFFImportFile.h:86
+  bool isWeak() {
+    if(ExtName.size() && Name != ExtName)
+      return true;
----------------
Please run clang-format-diff before uploading a patch.


================
Comment at: include/llvm/Object/COFFImportFile.h:87-88
+    if(ExtName.size() && Name != ExtName)
+      return true;
+    return false;
+  }
----------------
  if (foo)
    return true;
  return false;

is the same as

  return foo;


================
Comment at: include/llvm/Object/COFFModuleDefinition.h:45
+parseCOFFModuleDefinition(MemoryBufferRef MB, COFF::MachineTypes Machine,
+                          bool MingwDef = false);
 
----------------
I don't like to add a magical boolean flag to slightly change the existing behavior. Why do you need this? This should be removed unless absolutely necessary.


================
Comment at: lib/DlltoolDriver/CMakeLists.txt:10
+add_dependencies(LLVMDlltoolDriver DllOptionsTableGen)
\ No newline at end of file

----------------
Your file doesn't end with '\n'. Please fix.


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:103-131
+  SmallVector<const char *, 20> NewArgs(ArgsArr.begin(), ArgsArr.end());
+  BumpPtrAllocator Alloc;
+  StringSaver Saver(Alloc);
+  COFF::MachineTypes Machine = IMAGE_FILE_MACHINE_UNKNOWN;
+  cl::ExpandResponseFiles(Saver, cl::TokenizeWindowsCommandLine, NewArgs);
+  ArgsArr = NewArgs;
+
----------------
You can factor this out as a function that returns opt::InputArgList.


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:135
+  MemoryBufferRef MB;
+  int i=0;
+  for (auto *Arg : Args.filtered(OPT_INPUT)) {
----------------
clang-format-diff please.


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:135
+  MemoryBufferRef MB;
+  int i=0;
+  for (auto *Arg : Args.filtered(OPT_INPUT)) {
----------------
ruiu wrote:
> clang-format-diff please.
In LLVM coding style, all local variables should start with uppercase letter.


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:150
+
+  if(Machine == IMAGE_FILE_MACHINE_UNKNOWN) {
+    llvm::errs() << "unknown emulation mode\n";
----------------
ditto -- formatting errors are distracting which take reviewer's focus away from actual code. You really want to fix all mechanical errors before uploading new patches.


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:152
+    llvm::errs() << "unknown emulation mode\n";
+    return -2;
+  }
----------------
Why -2?


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:160
+    llvm::errs() << "error parsing definition\n" <<
+    errorToErrorCode(ImportCOFFMod.takeError()).message();
+    return -2;
----------------
Indentation


================
Comment at: lib/DlltoolDriver/DlltoolDriver.cpp:177
+
+  if(!import_error)
+    return 0;
----------------
Format error. Please fix everything using clang-format-diff.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892