[PATCH] D50404: [lld-link] Generalize handling of /debug and /debug:{none, full, fastlink, ghash, symtab}

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Driver.cpp:500
 
+enum class DebugGeneration {
+  Unknown,
----------------
Why does the name contani "Generation"?


================
Comment at: COFF/Driver.cpp:864
   // Handle /debug
-  if (Args.hasArg(OPT_debug, OPT_debug_dwarf, OPT_debug_ghash)) {
-    Config->Debug = true;
-    Config->Incremental = true;
-    if (auto *Arg = Args.getLastArg(OPT_debugtype))
-      Config->DebugTypes = parseDebugType(Arg->getValue());
-    else
-      Config->DebugTypes = getDefaultDebugType(Args);
-  }
-
-  // Handle /pdb
-  bool ShouldCreatePDB = Args.hasArg(OPT_debug, OPT_debug_ghash);
-  if (ShouldCreatePDB) {
-    if (auto *Arg = Args.getLastArg(OPT_pdb))
-      Config->PDBPath = Arg->getValue();
-    if (auto *Arg = Args.getLastArg(OPT_pdbaltpath))
-      Config->PDBAltPath = Arg->getValue();
-    if (Args.hasArg(OPT_natvis))
-      Config->NatvisFiles = Args.getAllArgValues(OPT_natvis);
+  DebugGeneration DebugGen = DebugGeneration::None;
+  bool ShouldCreatePDB = false;
----------------
It feels more complicated than before. I believe this piece of code can be written in a more flat style with less nesting. Maybe you could just dispatch based on a /debug value like this:

  StringRef S = Args.getLastArg(OPT_debug, OPT_debug_opt))
  if (S == "full") {
    Config->Debug = true;
    Config->Incremental = true;
    ShouldCreatePDB = true;
    ...
  } else if (S == "dwarf") {
    Config->Debug = true;
    Config->DebugTypes = DWARF;
    ...
  } else if ...



https://reviews.llvm.org/D50404