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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Please read your patch throughly again and again to fix all errors of the same kind until you think your patch is perfect. I think I'm pointing out the same errors probably too many times.



================
Comment at: lib/Object/COFFModuleDefinition.cpp:87-89
+      // This lets us handle "==" which is a dlltool ism.
+      if (Buf[1] == '=')
+        Buf = Buf.drop_front();
----------------
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, "=");


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:101
+  unsigned MissingCount;
+  llvm::opt::InputArgList Args =
+      Table.ParseArgs(ArgsArr.slice(1), MissingIndex, MissingCount);
----------------
Remove `llvm::`.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:129-131
+  std::string Path;
+  if (auto *Arg = Args.getLastArg(OPT_l))
+    Path = Arg->getValue();
----------------
  std::string Path = Args.getLastArgValue(OPT_l);


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:143
+  Expected<COFFModuleDefinition> Def =
+      parseCOFFModuleDefinition(MB, Machine, true);
+
----------------
You want to do this only when MB has contents, no?


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:155-156
+
+  if (Path.empty())
+    Path = getImplibPath(Def->OutputFile);
+
----------------
OK, so delete the above definition of `Path` and write everything here.

  std::string Path = Args.getLastArgValue(OPT_l);
  if (Path.empty())
    Path = getImplibPath(Def->OutputFile);


Repository:
  rL LLVM

https://reviews.llvm.org/D29892