[clang] NFC: Extract DiagnosticOptions parsing

[jansvoboda11]

Summary

The way we parse DiagnosticOptions is a bit involved.

DiagnosticOptions are parsed as part of the cc1-parsing function CompilerInvocation::CreateFromArgs which takes DiagnosticsEngine as an argument to be able to report errors in command-line arguments. But to create DiagnosticsEngine, DiagnosticOptions are needed. This is solved by exposing the ParseDiagnosticArgs to clients and making its DiagnosticsEngine argument optional, essentially breaking the dependency cycle.

The ParseDiagnosticArgs function takes llvm::opt::ArgList &, which each client needs to create from the command-line (typically represented as std::vector<const char *>). Creating this data structure in this context is somewhat particular. This code pattern is copy-pasted in some places across the upstream code base and also in downstream repos. To make things a bit more uniform, this patch extracts the code into a new reusable function: CreateAndPopulateDiagOpts.

[dexonsmith]

A few comments inline, but LGTM once you fix those.

clang/lib/Frontend/CompilerInvocation.cpp

2259–2260

Please return std::unique_ptr<DiagnosticOptions if this is an owning pointer. The caller can call .release if it needs to unpack it (although the current caller needn't do anything since there's an implicit conversion to IntrusiveRefCntPtr from std::unique_ptr).

Also, I'd prefer to propagate the lowerCamelCase style for functions recommended by the coding standards... up to you I guess since I know this file has a lot of UpperCamelCase.

2261

Please use std::make_unique<DiagnosticOptions>() instead of new.

2270–2274

I don't think -fintegrated-cc1 is related to diagnostic options. I suggest leaving it behind in the driver code and using:

std::unique_ptr<DiagnosticOptions>
clang::createAndPopulateDiagOpts(ArrayRef<const char *> Argv) {
  return createAndPopulateDiagOptsImpl(Argv);
}

[jansvoboda11]

Switch to unique_ptr, leave -fno-intergrated-cc1 behind.

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

2270–2274

I left the -fintegrated-cc1-related code in the function, since it works on the parsed InputArgList. The whole point of this function is to free its clients of parsing the command-line options into InputArgList. If we leave -fintegrated-cc1-related code in the driver, we'll need to parse the arguments into InputArgList twice (not a big fan of that), or iterate over the raw command-line manually (not a big fan of that either, but should be more efficient). I decided to leave the code in the driver and go with the second option. Let me know it that doesn't seem reasonable to you and I can revisit this.

clang/include/clang/Frontend/CompilerInvocation.h

 
 namespace clang {
 
 class DiagnosticsEngine;
 class HeaderSearchOptions;
 class PreprocessorOptions;
 class TargetOptions;
 
+// This lets us create the DiagnosticsEngine with a properly-filled-out
+// DiagnosticOptions instance.
+DiagnosticOptions *CreateAndPopulateDiagOpts(ArrayRef<const char *> Argv,
+                                             bool *UseNewCC1Process = nullptr);
+
 /// Fill out Opts based on the options given in Args.
 ///
 /// Args must have been created from the OptTable returned by
 /// createCC1OptTable().
 ///
 /// When errors are encountered, return false and, if Diags is non-null,
 /// report the error(s).
 bool ParseDiagnosticArgs(DiagnosticOptions &Opts, llvm::opt::ArgList &Args,

clang/lib/Frontend/CompilerInvocation.cpp

StringRef IgnoredRemarks[] = {"pass",          "no-pass",
                                   "pass-missed",   "no-pass-missed"};
     if (llvm::is_contained(IgnoredRemarks, Remark))
       continue;
 
     Args.push_back(SA(StringRef("-R") + Remark));
   }
 }
 
+DiagnosticOptions *clang::CreateAndPopulateDiagOpts(ArrayRef<const char *> Argv,
+                                                    bool *UseNewCC1Process) {
+  auto *DiagOpts = new DiagnosticOptions;
+  unsigned MissingArgIndex, MissingArgCount;
+  InputArgList Args = getDriverOptTable().ParseArgs(
+      Argv.slice(1), MissingArgIndex, MissingArgCount);
+  // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
+  // Any errors that would be diagnosed here will also be diagnosed later,
+  // when the DiagnosticsEngine actually exists.
+  (void)ParseDiagnosticArgs(*DiagOpts, Args);
+
+  if (UseNewCC1Process)
+    *UseNewCC1Process =
+        Args.hasFlag(clang::driver::options::OPT_fno_integrated_cc1,
+                     clang::driver::options::OPT_fintegrated_cc1,
+                     /*Default=*/CLANG_SPAWN_CC1);
+
+  return DiagOpts;
+}
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticsEngine *Diags,
                                 bool DefaultDiagColor) {
   Optional<DiagnosticsEngine> IgnoringDiags;
   if (!Diags) {
     IgnoringDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                           new IgnoringDiagConsumer());
     Diags = &*IgnoringDiags;

clang/lib/Interpreter/Interpreter.cpp

IncrementalCompilerBuilder::create(std::vector<const char *> &ClangArgv) {
   // Put a dummy C++ file on to ensure there's at least one compile job for the
   // driver to construct.
   ClangArgv.push_back("<<< inputs >>>");
 
   CompilerInvocation Invocation;
   // Buffer diagnostics from argument parsing so that we can output them using a
   // well formed diagnostic object.
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
+      CreateAndPopulateDiagOpts(ClangArgv);
   TextDiagnosticBuffer *DiagsBuffer = new TextDiagnosticBuffer;
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagsBuffer);
-  unsigned MissingArgIndex, MissingArgCount;
-  const llvm::opt::OptTable &Opts = driver::getDriverOptTable();
-  llvm::opt::InputArgList ParsedArgs =
-      Opts.ParseArgs(ArrayRef<const char *>(ClangArgv).slice(1),
-                     MissingArgIndex, MissingArgCount);
-  ParseDiagnosticArgs(*DiagOpts, ParsedArgs, &Diags);
 
   driver::Driver Driver(/*MainBinaryName=*/ClangArgv[0],
                         llvm::sys::getProcessTriple(), Diags);
   Driver.setCheckInputsExist(false); // the input comes from mem buffers
   llvm::ArrayRef<const char *> RF = llvm::makeArrayRef(ClangArgv);
   std::unique_ptr<driver::Compilation> Compilation(Driver.BuildCompilation(RF));
 
   if (Compilation->getArgs().hasArg(driver::options::OPT_v))

clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp

int main(int argc, const char **argv) {
 
   Args.push_back("-fsyntax-only");
   Args.push_back(Filename);
 
   std::vector<const char *> Argv(Args.size(), nullptr);
   llvm::transform(Args, Argv.begin(),
                   [](const std::string &Arg) { return Arg.c_str(); });
 
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  unsigned MissingArgIndex, MissingArgCount;
-  auto Opts = driver::getDriverOptTable();
-  auto ParsedArgs = Opts.ParseArgs(llvm::makeArrayRef(Argv).slice(1),
-                                   MissingArgIndex, MissingArgCount);
-  ParseDiagnosticArgs(*DiagOpts, ParsedArgs);
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
+      CreateAndPopulateDiagOpts(Argv);
 
   // Don't output diagnostics, because common scenarios such as
   // cross-compiling fail with diagnostics.  This is not fatal, but
   // just causes attempts to use the introspection API to return no data.
   TextDiagnosticPrinter DiagnosticPrinter(llvm::nulls(), &*DiagOpts);
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
       &DiagnosticPrinter, false);

clang/lib/Tooling/Tooling.cpp

if (OwnsAction)
     delete Action;
 }
 
 bool ToolInvocation::run() {
   std::vector<const char*> Argv;
   for (const std::string &Str : CommandLine)
     Argv.push_back(Str.c_str());
   const char *const BinaryName = Argv[0];
-  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  unsigned MissingArgIndex, MissingArgCount;
-  llvm::opt::InputArgList ParsedArgs = driver::getDriverOptTable().ParseArgs(
-      ArrayRef<const char *>(Argv).slice(1), MissingArgIndex, MissingArgCount);
-  ParseDiagnosticArgs(*DiagOpts, ParsedArgs);
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
+      CreateAndPopulateDiagOpts(Argv);
   TextDiagnosticPrinter DiagnosticPrinter(
       llvm::errs(), &*DiagOpts);
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
       DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
   // Although `Diagnostics` are used only for command-line parsing, the custom
   // `DiagConsumer` might expect a `SourceManager` to be present.
   SourceManager SrcMgr(Diagnostics, *Files);

clang/tools/driver/driver.cpp

static void FixupDiagPrefixExeName(TextDiagnosticPrinter *DiagClient,
   // If the clang binary happens to be named cl.exe for compatibility reasons,
   // use clang-cl.exe as the prefix to avoid confusion between clang and MSVC.
   StringRef ExeBasename(llvm::sys::path::stem(Path));
   if (ExeBasename.equals_insensitive("cl"))
     ExeBasename = "clang-cl";
   DiagClient->setPrefix(std::string(ExeBasename));
 }
 
-// This lets us create the DiagnosticsEngine with a properly-filled-out
-// DiagnosticOptions instance.
-static DiagnosticOptions *
-CreateAndPopulateDiagOpts(ArrayRef<const char *> argv, bool &UseNewCC1Process) {
-  auto *DiagOpts = new DiagnosticOptions;
-  unsigned MissingArgIndex, MissingArgCount;
-  InputArgList Args = getDriverOptTable().ParseArgs(
-      argv.slice(1), MissingArgIndex, MissingArgCount);
-  // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
-  // Any errors that would be diagnosed here will also be diagnosed later,
-  // when the DiagnosticsEngine actually exists.
-  (void)ParseDiagnosticArgs(*DiagOpts, Args);
-
-  UseNewCC1Process =
-      Args.hasFlag(clang::driver::options::OPT_fno_integrated_cc1,
-                   clang::driver::options::OPT_fintegrated_cc1,
-                   /*Default=*/CLANG_SPAWN_CC1);
-
-  return DiagOpts;
-}
-
 static void SetInstallDir(SmallVectorImpl<const char *> &argv,
                           Driver &TheDriver, bool CanonicalPrefixes) {
   // Attempt to find the original path used to invoke the driver, to determine
   // the installed path. We do this manually, because we want to support that
   // path being a symlink.
   SmallString<128> InstalledPath(argv[0]);
 
   // Do a PATH lookup, if there are no directory components.
int main(int Argc, const char **Argv) {
 
   // Whether the cc1 tool should be called inside the current process, or if we
   // should spawn a new clang subprocess (old behavior).
   // Not having an additional process saves some execution time of Windows,
   // and makes debugging and profiling easier.
   bool UseNewCC1Process;
 
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts =
-      CreateAndPopulateDiagOpts(Args, UseNewCC1Process);
+      CreateAndPopulateDiagOpts(Args, &UseNewCC1Process);
 
   TextDiagnosticPrinter *DiagClient
     = new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
   FixupDiagPrefixExeName(DiagClient, Path);
 
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
 
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient);