[PATCH] D33880: COFF: Introduce LD shim around LINK

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I forgot to mention that this patch needs a test. Please create a subdirectory MinGW under lld/tests and put a test file there for this driver.



================
Comment at: MinGW/Driver.cpp:97
+// This is for -lfoo. We'll look for libfoo.dll.a or libfoo.a from search paths.
+Optional<std::string> searchLibrary(StringRef Name, bool IgnoreShared = false) {
+  if (Name.startswith(":"))
----------------
martell wrote:
> ruiu wrote:
> > You are using this function only once, and at that call site `IgnoreShared` is not passed, so it is always false. Please remove that parameter.
> It is based on the `-Bstatic` flag.
> 
> `addLibrary(Arg->getValue(), Args.hasArg(OPT_Bstatic));`
> which in turn calls
> `searchLibrary(Name, StaticOnly))`
> passing in the bool
You can at least remove `= false`. It is nice to keep the variable name, so I'd rename this StaticOnly.


================
Comment at: MinGW/Driver.cpp:190
+  std::vector<const char *> LinkVec(LinkArgs.size(), nullptr);
+  for (std::vector<std::string>::size_type I = 0; I < LinkArgs.size(); ++I) {
+    LinkVec[I] = LinkArgs[I].c_str();
----------------
martell wrote:
> I could probably use `auto` here instead of `std::vector<std::string>::size_type`?
> Other then that I think I have addressed all the comments
I think just `size_t` suffices. However, probably the best way of doing this is something like this:

  std::vector<const char *> Vec;
  for (const std::string &S : LinkArgs)
    Vec.push_back(S.c_str());
  return coff::link(Vec);


================
Comment at: MinGW/Driver.cpp:128-129
+
+static void forward(opt::InputArgList &Args, unsigned Key, StringRef OutArg,
+                    const char *Default = nullptr) {
+  StringRef S = Args.getLastArgValue(Key);
----------------
  StringRef OutArg -> const std::string &OutArg

to remove `Twine` and `str()`.

You generally want to avoid C strings in C++, so `const char *Default = nullptr` -> `std::string Default = ""`.


================
Comment at: MinGW/Driver.cpp:131
+  StringRef S = Args.getLastArgValue(Key);
+  if (!S.empty()) {
+    LinkArgs.push_back(Twine("-" + OutArg + ":" + S).str());
----------------
For consistency with `addLibrary`, remove {}.


================
Comment at: MinGW/Driver.cpp:139
+static void forwardValue(opt::InputArgList &Args, unsigned Key,
+                         StringRef CmpArg, StringRef OutArg) {
+  StringRef S = Args.getLastArgValue(Key);
----------------
If you change `StringRef OutArg` to `const std::string &OutArg`, then you can remove `Twine` and `str()`.


================
Comment at: MinGW/Driver.cpp:170-174
+  forward(Args, OPT_entry, "ENTRY");
+  forward(Args, OPT_subs, "SUBSYSTEM");
+  forward(Args, OPT_shared, "DLL");
+  forward(Args, OPT_outlib, "IMPLIB");
+  forward(Args, OPT_stack, "STACK");
----------------
I'd make them lowercase as well.


================
Comment at: MinGW/Driver.cpp:182-183
+  // handle __image_base__
+  auto M = Args.getLastArg(OPT_m);
+  if (M && StringRef(M->getValue()) == "i386pe")
+    LinkArgs.push_back("/alternatename:__image_base__=___ImageBase");
----------------
  if (Args.getLastArgValue(OPT_m) == "i386pe")


================
Comment at: MinGW/Options.td:35-38
+
+// Style
+def rsp_quoting: J<"rsp-quoting=">,
+  HelpText<"Quoting style for response files. Values supported: windows|posix">;
----------------
You are not handling this one, so please remove.


Repository:
  rL LLVM

https://reviews.llvm.org/D33880