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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: MinGW/Driver.cpp:53
+// Create table mapping all options defined in Options.td
+static const opt::OptTable::Info infoTable[] = {
+#define OPTION(X1, X2, ID, KIND, GROUP, ALIAS, X7, X8, X9, X10, X11, X12)      \
----------------
mstorsjo wrote:
> martell wrote:
> > ruiu wrote:
> > > infoTable -> InfoTable
> > > 
> > > (Yet another style issue, and honestly I'm a bit tired of pointing them out, so please please fix them before sending your patch to review. If you do not pay attention to the documented and obvious coding style rules, I would probably suspend doing "real" code review and instead pointing out only style issues until your code follows the coding style. I understand that we cannot and do not want to reduce the error rate to zero, and I really appreciate your work on lld/mingw, but pointing out that variables are in CamelCase in the LLVM coding style on virtually every patch update is a waste of both your and my time.)
> > I addressed ALL the CamelCase variables in the patch and left a note for this one specifically before you reviewed it again. You should see a comment a few lines below where `infoTable` is used with the following text
> > `infoTable doesn't seem to follow the variable naming guide but this is done consistently across LLVM`
> > 
> > Here are references to the locations I found using this exact spelling which is why I left it.
> > 
> > LLD
> > 
> > COFF/DriverUtils.cpp
> > lib/Driver/DarwinLdDriver.cpp
> > 
> > LLVM
> > 
> > tools/llvm-mt/llvm-mt.cpp
> > tools/llvm-rc/llvm-rc.cpp
> > lib/ToolDrivers/llvm-lib/LibDriver.cpp
> > tools/llvm-cvtres/llvm-cvtres.cpp
> > unittests/Option/OptionParsingTest.cpp
> > etc
> > 
> > Do you still want me to change it?
> I understand you frustration Rui, I truly do (especially given earlier revisions of this and other patches), but this one was surely clearly pointed out by Martell before - unfortunately I guess the comment was buried in the log and not at the right spot in the inline diff.
> 
> If we give Martell the benefit of doubt that it is ok wrt to variable naming (since he did point this one out explicitly before with a reasoning) and other style issues I assume he's fixed with clang-format by now, what's the status of functional review of this patch? Is it almost done with a few minor issues yet to be found, or has that not been done thoroughly yet at all due to major style issues in all the previous iterations?
I'm sorry that I missed your previous comment, Martell. But `infoTable`s in many files are just copypasta and shouldn't be intentional.

As to the functionality, I think this patch is towards a right direction, though there are a few things that have to be addressed.


================
Comment at: MinGW/Driver.cpp:71
+StringSaver Saver{BAlloc};
+static SmallVector<const char *, 256> LinkArgs;
+static std::vector<StringRef> SearchPaths;
----------------
I'd change the type of this variable to `std::vector<std::string>` and repack to `std::vector<const char *>` at end of `link()`. This should eliminate a lot of uses of `Saver.save()`.


================
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(":"))
----------------
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.


================
Comment at: MinGW/Driver.cpp:120
+  for (auto *Arg : Args) {
+    switch (Arg->getOption().getID()) {
+    case OPT_l:
----------------
Use `Arg->getOption().getUnaliasedOption().getID()` instead of `Arg->getOption().getID()` so that we don't need to worry about aliases.


================
Comment at: MinGW/Driver.cpp:125
+    case OPT_INPUT:
+      LinkArgs.push_back(Saver.save(Arg->getValue()).data());
+      break;
----------------
Do you really have to call Saver.save here? The use of Saver.save is not consistent in this patch, and it might be saving a string that's already been saved.

I believe we should remove Saver entirely from this patch. Can you do that if you replace some occurrences of StringRef with std::strings in this patch, no?


================
Comment at: MinGW/Driver.cpp:131
+
+static std::vector<StringRef> getArgs(opt::InputArgList &Args, int Id) {
+  std::vector<StringRef> V;
----------------
You are using this function only once so inline this.


================
Comment at: MinGW/Driver.cpp:141
+  StringRef S = Args.getLastArgValue(Key);
+  if (S.size()) {
+    LinkArgs.push_back(Saver.save(Twine("-" + OutArg + ":" + S)).data());
----------------
By convention, use `empty` instead of `size`.


================
Comment at: MinGW/Driver.cpp:150-151
+static void forwardValue(opt::InputArgList &Args, unsigned Key,
+                         StringRef CmpArg, StringRef OutArg,
+                         StringRef OutValueArg) {
+  StringRef S = Args.getLastArgValue(Key);
----------------
Merge OutArg and OutValueArg and do that at caller side. For example, instead of doing

  forwardValue(Args, OPT_m, "i386pe", "MACHINE", "X86");

, you can just do

  forwardValue(Args, OPT_m, "i386pe", "MACHINE:X86");

Also, lowercase options are probably preferred, so it's even better if you do

  forwardValue(Args, OPT_m, "i386pe", "machine:x86");



================
Comment at: MinGW/Driver.cpp:189
+
+  SearchPaths = getArgs(Args, OPT_L);
+  createFiles(Args);
----------------
  for (auto *Arg : Args.filtered(OPT_L))
    SearchPaths.push_back(Arg->getValue());


Repository:
  rL LLVM

https://reviews.llvm.org/D33880