[PATCH] D45771: [Driver] Support for -save-stats in AddGoldPlugin.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added a comment.

Some cleanup suggestions included, but I like the change overall.



================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:1250
+  // Setup statistics file output.
+  if (const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ)) {
+    StringRef SaveStats = A->getValue();
----------------
Early return would be nicer.

```
const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
if (!A)
  return {};
```


================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:1264
+    } else {
+      D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+    }
----------------
Early return after the check would allow you to avoid the boolean and reduce indentation below too.

```
if (SaveStats != "cwd" && SaveStats != "obj") {
  D.Diag(drag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
  return {};
}
```


================
Comment at: lib/Driver/ToolChains/CommonArgs.h:65
                    llvm::opt::ArgStringList &CmdArgs, bool IsThinLTO,
                    const Driver &D);
 
----------------
While you're here, perhaps you can improve this a slight bit more?

`D` is unnecessary, `ToolChain.getDriver()` allows you to get the value anyways.  I think it would be nicer to move the `Input` and `Output` parameters to before the `IsThinLTO`.


Repository:
  rC Clang

https://reviews.llvm.org/D45771