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

[Martell Malone via Phabricator via llvm-commits]

martell added inline comments.


================
Comment at: lib/Object/COFFModuleDefinition.cpp:87-89
+      // This lets us handle "==" which is a dlltool ism.
+      if (Buf[1] == '=')
+        Buf = Buf.drop_front();
----------------
ruiu wrote:
> This is not safe because Buf may contain just one character (and if that's the case, accessing Buf[1] can cause a runtime error.) You want to do this.
> 
>   Buf = Buf.drop_front();
>   // GNU dlltool accepts both = and ==.
>   if (Buf.startswith('='))
>     Buf = Buf.drop_front();
>   return Token(Equal, "=");
I suppose we could have a bad user that does something like `hellofunc =` without finishing the line.
Will do startswith but with a NULL terminated StringRef instead of a `char`.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:101
+  unsigned MissingCount;
+  llvm::opt::InputArgList Args =
+      Table.ParseArgs(ArgsArr.slice(1), MissingIndex, MissingCount);
----------------
ruiu wrote:
> Remove `llvm::`.
This is not consistent with the rest of the llvm project.
Anywhere `opt` is used regardless of `using namespace llvm;` it is displayed as `llvm::opt`
Similar to `llvm::errs` and `llvm::outs`
For example `llvm::opt::OptTable` is mentioned in this file twice and every other driver.
Just looking at the lib driver I can see `llvm::InputArgList` in main.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:143
+  Expected<COFFModuleDefinition> Def =
+      parseCOFFModuleDefinition(MB, Machine, true);
+
----------------
ruiu wrote:
> You want to do this only when MB has contents, no?
Added a check after the creation of MB to say `definition file is empty` in that case.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892