[PATCH] D29892: ar: Add new driver to support dlltool

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Before taking a look at details, I have a question. This change copies code from LLD to LLVM, but can you move the code instead? If it is a part of the plan and will be done in a follow-up patch, that's OK, but having duplicate code isn't good.



================
Comment at: include/llvm/DllDriver/DllDriver.h:1
+//===- llvm/DllDriver/DllDriver.h - dlltool.exe-compatible driver ---*- C++ -*-===//
+//
----------------
Probably it should be DlltoolDriver as not "dll" but "dlltool" is the name of the tool.


================
Comment at: include/llvm/DllDriver/DllDriver.h:21
+
+int dllToolDriverMain(ArrayRef<const char *> ARgs);
+}
----------------
dlltool is a one word, so `dlltoolDriverMain`.

ARgs?


================
Comment at: lib/DllDriver/Config.h:1
+#ifndef LLVM_DLLDRIVER_CONFIG_H
+#define LLVM_DLLDRIVER_CONFIG_H
----------------
This is a direct translation of LLD, but it is too LLD-ish and not good for code that lives in llvm/lib. You want to handle errors as return values using `Expected` or `Error`, for example. You don't want to have a global variable "Configulation".


Repository:
  rL LLVM

https://reviews.llvm.org/D29892