[PATCH] D37709: [MinGW] Add support for the options --[no-]whole-archive

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking good.

Could you split this patch into two, one for COFF and one for MinGW?



================
Comment at: COFF/Driver.cpp:135
+    }
     return Symtab->addFile(make<ArchiveFile>(MBRef));
+  }
----------------
It's not new code, but can you write it in two lines?

  Symtab->addFile(make<ArchiveFile>(MBRef));
  return;

Because `addFile` returns void and this function returns void too, it is legal to write `return Symtab->addFile(...)`, but this is still an odd feature of C++, and I believe we generally want to avoid this unless it's generated as instantiated templates.


================
Comment at: COFF/Driver.cpp:137
+  }
   if (Magic == file_magic::bitcode)
     return Symtab->addFile(make<BitcodeFile>(MBRef));
----------------
nit: add a blank line befor ethis line.


================
Comment at: COFF/Driver.cpp:138
   if (Magic == file_magic::bitcode)
     return Symtab->addFile(make<BitcodeFile>(MBRef));
 
----------------
Likewise, separate into two lines.


================
Comment at: COFF/Driver.cpp:975
+      if (Optional<StringRef> Path = findFile(Arg->getValue()))
+        enqueuePath(*Path, WholeArchive);
+      break;
----------------
Replacing `WholeArchive` with `Args.hasArg(OPT_wholearchive_flag)` might improve readability.


================
Comment at: MinGW/Driver.cpp:175
 
-  for (auto *A : Args.filtered(OPT_INPUT, OPT_l)) {
-    if (A->getOption().getUnaliasedOption().getID() == OPT_INPUT)
-      Add(A->getValue());
-    else
-      Add(searchLibrary(A->getValue(), SearchPaths, Args.hasArg(OPT_Bstatic)));
+  StringRef WholeArchivePrefix = "";
+  for (auto *A : Args.filtered(OPT_INPUT, OPT_l, OPT_whole_archive,
----------------
I think I'd name this just `Prefix` as we generally prefer shorter variable names in lld.


================
Comment at: MinGW/Driver.cpp:183
+    case OPT_l:
+      Add(WholeArchivePrefix + searchLibrary(A->getValue(), SearchPaths, Args.hasArg(OPT_Bstatic)));
+      break;
----------------
80 cols (please run clang-format if you have installed it.)


https://reviews.llvm.org/D37709