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

[Will Wilson via Phabricator via llvm-commits]

lantictac added inline comments.


================
Comment at: COFF/Driver.cpp:500
 
+enum class DebugGeneration {
+  Unknown,
----------------
ruiu wrote:
> Why does the name contani "Generation"?
This was based on the MSDN docs as DebugType was already taken. I've changed to DebugKind in the hope it makes a little more sense.


================
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;
----------------
ruiu wrote:
> 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 ...
> 
Agreed. I've attempted to flatten the code a little in the latest patch.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50404