[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:521
+      .Case("symtab", DebugKind::Symtab)
+      .Default(DebugKind::Unknown);
+}
----------------
Let's report an error inside this function and return a dummy kind (e.g. DebugKind::None) for an unknown value, so that the caller doesn't have to care about the error condition.


================
Comment at: COFF/Driver.cpp:550
+    return parseDebugType(Arg->getValue());
+  else
+    return getDefaultDebugType(Args);
----------------
nit: no `else` after `return`.


================
Comment at: COFF/Driver.cpp:551
+  else
+    return getDefaultDebugType(Args);
+}
----------------
If `getDefaultDebugType` is called by this line, I'd inline that function here.


================
Comment at: COFF/Driver.cpp:915
+  DebugKind DK = DebugKind::None;
+  if (auto *DebugArg = Args.getLastArg(OPT_debug, OPT_debug_opt)) {
+    DK = DebugKind::Full;
----------------
DebugArg -> A for consistency.


================
Comment at: COFF/Driver.cpp:920
+      if (DK == DebugKind::FastLink) {
+        warn("/debug:fastlink unsupported; using /debug:full");
+        DK = DebugKind::Full;
----------------
Let's do this inside `parseDebugKind`.


================
Comment at: COFF/Options.td:93
 
-def debug : F<"debug">, HelpText<"Embed a symbol table in the image">;
-def debug_full : F<"debug:full">, Alias<debug>;
+defm debug : POpt<"debug", "Embed a symbol table in the image">;
 def debugtype : P<"debugtype", "Debug Info Options">;
----------------
Since this is the only place where you are using `POpt`, I wouldn't define that multiclass. Instead, I'd jsut define `debug` and `debug_opt` right here.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50404