[PATCH] D56392: [LLD][COFF] Support /ignore:4099. Support /ignore with comma-separated arguments.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Driver.cpp:991
+    while (!Curr.empty()) {
+      auto It = Curr.split(',');
+      if (It.first == "4037")
----------------
What `StringRef::split` returns is not an iterator, so I think `It` is not a very good name. Since this is not a performance-critical path, maybe this is the simplest code I could think of:

  SmallVector<StringRef, 1> Vec;
  StringRef(Arg->getValue())->split(Vec, ',');
  for (StringRef S : Vec) {
    if (S == "4037") {
      ...

You can find the same pattern in the code handling `OPT_opt`.


================
Comment at: COFF/PDB.cpp:1304
   if (!IndexMapResult) {
-    auto FileName = sys::path::filename(Path);
-    warn("Cannot use debug info for '" + FileName + "'\n" +
-         ">>> failed to load reference " +
-         StringRef(toString(IndexMapResult.takeError())));
+    if (Config->WarnDebugInfoUnusable) {
+      auto FileName = sys::path::filename(Path);
----------------
Flip the condition to return early.


================
Comment at: COFF/PDB.cpp:1305
+    if (Config->WarnDebugInfoUnusable) {
+      auto FileName = sys::path::filename(Path);
+      warn("Cannot use debug info for '" + FileName + "' [LNK4099]\n" +
----------------
Please avoid using `auto` unless its type is obvious from the right-hand side (for example, if the rhs is a `dyn_cast`, then `auto` is fine, but in this case the actual type is not obvious).


================
Comment at: COFF/PDB.cpp:1306
+      auto FileName = sys::path::filename(Path);
+      warn("Cannot use debug info for '" + FileName + "' [LNK4099]\n" +
+           ">>> failed to load reference " +
----------------
I don't know if adding `[LNK4099]` is the right thing. We have a lot of warnings and error messages in lld, but no one includes such error code. If we want to do this (I personally don't want though), we should do for everything, but this needs to be discussed first.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56392/new/

https://reviews.llvm.org/D56392