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

[Martell Malone via Phabricator via llvm-commits]

martell marked 2 inline comments as done.
martell added a comment.

In https://reviews.llvm.org/D29892#712675, @ruiu wrote:

> 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.


I had planned to do it in a follow-up patch which just removes it from lld and just calls `createImportLibrary` or if you would prefer we can call `llvm-lib` directly,
I'm sure you have a better understanding of what is best there.
I use `git-svn` and doing it all as one commit for the sub repos seem awkward.
On that I left a comment about the parser to reduce duplication also.



================
Comment at: lib/DllDriver/Config.h:1
+#ifndef LLVM_DLLDRIVER_CONFIG_H
+#define LLVM_DLLDRIVER_CONFIG_H
----------------
ruiu wrote:
> 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".
Makes sense.
I will have a look at using `Expected` or `Error`

For the `Configuration` it might be best to add to the parser and have that as part of llvm also because it can mostly be reused for `llvm-lib`

Have you any suggestions on where this would live?


Repository:
  rL LLVM

https://reviews.llvm.org/D29892