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

[Martell Malone via Phabricator via llvm-commits]

martell marked an inline comment as done.
martell added inline comments.


================
Comment at: lib/Object/ArchiveWriter.cpp:322
+      if (Symflags & object::SymbolRef::SF_Undefined &&
+        !(Symflags & object::SymbolRef::SF_Indirect))
         continue;
----------------
ruiu wrote:
> Indentation. Please use clang-format-diff to format your patch before requesting code review.
Sorry I thought the indentation of 2 spaces is correct already.
It actually wraps it on the next line to the external bracket.


================
Comment at: lib/Object/COFFModuleDefinition.cpp:222-227
     if (Machine == IMAGE_FILE_MACHINE_I386) {
-      if (!isDecorated(E.Name))
+      if (!isDecorated(E.Name, MingwDef))
         E.Name = (std::string("_").append(E.Name));
-      if (!E.ExtName.empty() && !isDecorated(E.ExtName))
+      if (!E.ExtName.empty() && !isDecorated(E.ExtName, MingwDef))
         E.ExtName = (std::string("_").append(E.ExtName));
     }
----------------
ruiu wrote:
> Do you think you can just do this?
> 
>     if (Machine == IMAGE_FILE_MACHINE_I386 && !MingwDef) {
>       if (!isDecorated(E.Name))
>         E.Name = (std::string("_").append(E.Name));
>       if (!E.ExtName.empty() && !isDecorated(E.ExtName))
>         E.ExtName = (std::string("_").append(E.ExtName));
>     }
> 
No because `@` and `?` are are still valid decorators for mingw


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:53
+static const llvm::opt::OptTable::Info infoTable[] = {
+#define OPTION(X1, X2, ID, KIND, GROUP, ALIAS, X6, X7, X8, X9, X10, X11)       \
+  {X1, X2, X9,          X10,         OPT_##ID, llvm::opt::Option::KIND##Class, \
----------------
ruiu wrote:
> There's a off-by-one error. X6 should be X7 and so on.
Look at `lib/ToolDrivers/llvm-lib/LibDriver.cpp#L44`
I can change it but every infoTable in the llvm source tree has this off by 1 for the #define



================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:112-115
+    llvm::errs() << "missing arg value for \""
+                 << Args.getArgString(MissingIndex) << "\", expected "
+                 << MissingCount
+                 << (MissingCount == 1 ? " argument.\n" : " arguments.\n");
----------------
ruiu wrote:
> Let's simplify this.
> 
>   llvm::errs() << Args.getArgString(MissingIndex) + ": missing argument\n";
assuming `<<` and not `+`


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:128
+    Table.PrintHelp(outs(), ArgsArr[0], "dlltool", false);
+    outs() << "\nTARGETS: ";
+    printTargets();
----------------
ruiu wrote:
> It is easier to just add `i386, i386:x86-64, arm` at end of this line
Thought it would be better to have that list in once place but that works also. 


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:141
+  for (auto *Arg : Args.filtered(OPT_UNKNOWN))
+    llvm::errs() << "ignoring unknown argument: " << Arg->getSpelling() << "\n";
+
----------------
ruiu wrote:
> Remove `llvm::` as you did `using namespace llvm`.
This is all based on the rest of the source code of llvm.
`llvm::errs` seems to be used throughout for clarity over `errs()`
Just looking at `lib/ToolDrivers/llvm-lib/LibDriver.cpp` or any other source file in llvm I can see it follows this pattern.
We shouldn't be too pedantic about this.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892