[NFC][clang] Move much of the argument handling code from Driver::BuildActions to Driver::handleArguments.

[plotfi]

Summary

This change will go in on top of https://reviews.llvm.org/D65993.
It simply moves the code that already exists into a new function.

[plotfi]

really sorry for how bad this diff looks. phab just doesnt present in a very ideal way.

[aaron.ballman]

clang/include/clang/Driver/Driver.h

264

I think the presence of the YcArg and YuArg parameters needs some explanation in the comments, as those seem rather out of place.

[plotfi]

clang/include/clang/Driver/Driver.h

264

Those are the only corner case as far as things used and modified in the scope. Was just trying to preserve the existing behavior. I think those two args are used for clang-cl and the code that handles them just sets them to nullptr. I can add a comment trying to explain what is happening here. There were some tests that I remember were failing if those args weren't set to nullptr after being used.

[plotfi]

Moved the initialization of YcArg and YuArg and initial handling of those args into handleArguments. ninja check-clang passes.

[plotfi]

clang/include/clang/Driver/Driver.h

264

I got rid of those arguments. Moved the code that starts setting those and using those variables into handleArguments. That makes sense to me since that code is handling the clang-cl /Yu and /Yc args.

[plotfi]

@aaron.ballman Good catch on the /Yc /Yu args. I like the way its looking a lot more now.

[compnerd]

Ugh, this is still not the most structured handling of the arguments. But, yeah, this seems like it should be equivalent. Fine by me if @aaron.ballman has no more comments.

[aaron.ballman]

LGTM!

clang/include/clang/Driver/Driver.h

 #include "clang/Basic/LLVM.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Phases.h"
 #include "clang/Driver/ToolChain.h"
 #include "clang/Driver/Types.h"
 #include "clang/Driver/Util.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/StringSaver.h"
 
 #include <list>
 #include <map>
 #include <string>
 
 namespace llvm {

 private:
   /// TranslateInputArgs - Create a new derived argument list from the input
   /// arguments, after applying the standard argument translations.
   llvm::opt::DerivedArgList *
   TranslateInputArgs(const llvm::opt::InputArgList &Args) const;
 
   // getFinalPhase - Determine which compilation mode we are in and record
   // which option we used to determine the final phase.
+  // TODO: Much of what getFinalPhase returns are not actually true compiler
+  //       modes. Fold this functionality into Types::getCompilationPhases and
+  //       handleArguments.
   phases::ID getFinalPhase(const llvm::opt::DerivedArgList &DAL,
                            llvm::opt::Arg **FinalPhaseArg = nullptr) const;
 
+  // handleArguments - All code related to claiming and printing diagnostics
+  // related to arguments to the driver are done here.
+  void handleArguments(Compilation &C, llvm::opt::DerivedArgList &Args,
+                       const InputList &Inputs, ActionList &Actions,
+                       llvm::opt::Arg *YcArg, llvm::opt::Arg *YuArg) const;
+
   // Before executing jobs, sets up response files for commands that need them.
   void setUpResponseFiles(Compilation &C, Command &Cmd);
 
   void generatePrefixedToolNames(StringRef Tool, const ToolChain &TC,
                                  SmallVectorImpl<std::string> &Names) const;
 
   /// Find the appropriate .crash diagonostic file for the child crash
   /// under this driver and copy it out to a temporary destination with the

clang/lib/Driver/Driver.cpp

Action *processHostLinkAction(Action *HostAction) {
     OffloadAction::HostDependence HDep(
         *HostAction, *C.getSingleOffloadToolChain<Action::OFK_Host>(),
         /*BoundArch*/ nullptr, ActiveOffloadKinds);
     return C.MakeAction<OffloadAction>(HDep, DDeps);
   }
 };
 } // anonymous namespace.
 
-void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
-                          const InputList &Inputs, ActionList &Actions) const {
-  llvm::PrettyStackTraceString CrashInfo("Building compilation actions");
-
-  if (!SuppressMissingInputWarning && Inputs.empty()) {
-    Diag(clang::diag::err_drv_no_input_files);
-    return;
-  }
-
-  // Reject -Z* at the top level, these options should never have been exposed
-  // by gcc.
-  if (Arg *A = Args.getLastArg(options::OPT_Z_Joined))
-    Diag(clang::diag::err_drv_use_of_Z_option) << A->getAsString(Args);
-
-  // Diagnose misuse of /Fo.
-  if (Arg *A = Args.getLastArg(options::OPT__SLASH_Fo)) {
-    StringRef V = A->getValue();
-    if (Inputs.size() > 1 && !V.empty() &&
-        !llvm::sys::path::is_separator(V.back())) {
-      // Check whether /Fo tries to name an output file for multiple inputs.
-      Diag(clang::diag::err_drv_out_file_argument_with_multiple_sources)
-          << A->getSpelling() << V;
-      Args.eraseArg(options::OPT__SLASH_Fo);
-    }
-  }
-
-  // Diagnose misuse of /Fa.
-  if (Arg *A = Args.getLastArg(options::OPT__SLASH_Fa)) {
-    StringRef V = A->getValue();
-    if (Inputs.size() > 1 && !V.empty() &&
-        !llvm::sys::path::is_separator(V.back())) {
-      // Check whether /Fa tries to name an asm file for multiple inputs.
-      Diag(clang::diag::err_drv_out_file_argument_with_multiple_sources)
-          << A->getSpelling() << V;
-      Args.eraseArg(options::OPT__SLASH_Fa);
-    }
-  }
-
-  // Diagnose misuse of /o.
-  if (Arg *A = Args.getLastArg(options::OPT__SLASH_o)) {
-    if (A->getValue()[0] == '\0') {
-      // It has to have a value.
-      Diag(clang::diag::err_drv_missing_argument) << A->getSpelling() << 1;
-      Args.eraseArg(options::OPT__SLASH_o);
-    }
-  }
-
-  // Ignore /Yc/Yu if both /Yc and /Yu passed but with different filenames.
-  Arg *YcArg = Args.getLastArg(options::OPT__SLASH_Yc);
-  Arg *YuArg = Args.getLastArg(options::OPT__SLASH_Yu);
-  if (YcArg && YuArg && strcmp(YcArg->getValue(), YuArg->getValue()) != 0) {
-    Diag(clang::diag::warn_drv_ycyu_different_arg_clang_cl);
-    Args.eraseArg(options::OPT__SLASH_Yc);
-    Args.eraseArg(options::OPT__SLASH_Yu);
-    YcArg = YuArg = nullptr;
-  }
-  if (YcArg && Inputs.size() > 1) {
-    Diag(clang::diag::warn_drv_yc_multiple_inputs_clang_cl);
-    Args.eraseArg(options::OPT__SLASH_Yc);
-    YcArg = nullptr;
-  }
-
-  // Builder to be used to build offloading actions.
-  OffloadingActionBuilder OffloadBuilder(C, Args, Inputs);
-
-  // Construct the actions to perform.
-  HeaderModulePrecompileJobAction *HeaderModuleAction = nullptr;
-  ActionList LinkerInputs;
-
-  {
-    Arg *FinalPhaseArg;
-    phases::ID FinalPhase = getFinalPhase(Args, &FinalPhaseArg);
+void Driver::handleArguments(Compilation &C, DerivedArgList &Args,
+                             const InputList &Inputs, ActionList &Actions,
+                             Arg *YcArg, Arg *YuArg) const {
 
-    if (FinalPhase == phases::Link) {
-      if (Args.hasArg(options::OPT_emit_llvm))
-        Diag(clang::diag::err_drv_emit_llvm_link);
-      if (IsCLMode() && LTOMode != LTOK_None &&
-          !Args.getLastArgValue(options::OPT_fuse_ld_EQ).equals_lower("lld"))
-        Diag(clang::diag::err_drv_lto_without_lld);
-    }
 
-    if (FinalPhase == phases::Preprocess ||
-        Args.hasArg(options::OPT__SLASH_Y_)) {
-      // If only preprocessing or /Y- is used, all pch handling is disabled.
-      // Rather than check for it everywhere, just remove clang-cl pch-related
-      // flags here.
-      Args.eraseArg(options::OPT__SLASH_Fp);
-      Args.eraseArg(options::OPT__SLASH_Yc);
-      Args.eraseArg(options::OPT__SLASH_Yu);
-      YcArg = YuArg = nullptr;
-    }
+  if (FinalPhase == phases::Preprocess || Args.hasArg(options::OPT__SLASH_Y_)) {
 
-    unsigned LastPLSize = 0;
-    for (auto &I : Inputs) {
-      types::ID InputType = I.first;
-      const Arg *InputArg = I.second;
 
-      llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PL;
-      types::getCompilationPhases(InputType, PL);
-      LastPLSize = PL.size();
 
-      // If the first step comes after the final phase we are doing as part of
-      // this compilation, warn the user about it.
-      phases::ID InitialPhase = PL[0];
-      if (InitialPhase > FinalPhase) {
-        if (InputArg->isClaimed())
-          continue;
 
-        // Claim here to avoid the more general unused warning.
-        InputArg->claim();
 
-        // Suppress all unused style warnings with -Qunused-arguments
-        if (Args.hasArg(options::OPT_Qunused_arguments))
-          continue;
 
-        // Special case when final phase determined by binary name, rather than
-        // by a command-line argument with a corresponding Arg.
-        if (CCCIsCPP())
-          Diag(clang::diag::warn_drv_input_file_unused_by_cpp)
-              << InputArg->getAsString(Args) << getPhaseName(InitialPhase);
-        // Special case '-E' warning on a previously preprocessed file to make
-        // more sense.
-        else if (InitialPhase == phases::Compile &&
-                 (Args.getLastArg(options::OPT__SLASH_EP,
-                                  options::OPT__SLASH_P) ||
-                  Args.getLastArg(options::OPT_E) ||
-                  Args.getLastArg(options::OPT_M, options::OPT_MM)) &&
-                 getPreprocessedType(InputType) == types::TY_INVALID)
-          Diag(clang::diag::warn_drv_preprocessed_input_file_unused)
-              << InputArg->getAsString(Args) << !!FinalPhaseArg
-              << (FinalPhaseArg ? FinalPhaseArg->getOption().getName() : "");
-        else
-          Diag(clang::diag::warn_drv_input_file_unused)
-              << InputArg->getAsString(Args) << getPhaseName(InitialPhase)
-              << !!FinalPhaseArg
-              << (FinalPhaseArg ? FinalPhaseArg->getOption().getName() : "");
-        continue;
-      }
 
-      if (YcArg) {
-        // Add a separate precompile phase for the compile phase.
-        if (FinalPhase >= phases::Compile) {
-          const types::ID HeaderType = lookupHeaderTypeForSourceType(InputType);
-          llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PCHPL;
-          types::getCompilationPhases(HeaderType, PCHPL);
-          // Build the pipeline for the pch file.
-          Action *ClangClPch = C.MakeAction<InputAction>(*InputArg, HeaderType);
-          for (phases::ID Phase : PCHPL)
-            ClangClPch = ConstructPhaseAction(C, Args, Phase, ClangClPch);
-          assert(ClangClPch);
-          Actions.push_back(ClangClPch);
-          // The driver currently exits after the first failed command.  This
-          // relies on that behavior, to make sure if the pch generation fails,
-          // the main compilation won't run.
-          // FIXME: If the main compilation fails, the PCH generation should
-          // probably not be considered successful either.
-        }
-      }
-    }
 
-    // If we are linking, claim any options which are obviously only used for
-    // compilation.
-    // FIXME: Understand why the last Phase List length is used here.
-    if (FinalPhase == phases::Link && LastPLSize == 1) {
-      Args.ClaimAllArgs(options::OPT_CompileOnly_Group);
-      Args.ClaimAllArgs(options::OPT_cl_compile_Group);
-    }
-  }
 
+void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
+                          const InputList &Inputs, ActionList &Actions) const {
+  llvm::PrettyStackTraceString CrashInfo("Building compilation actions");
+
+  if (!SuppressMissingInputWarning && Inputs.empty()) {
+    Diag(clang::diag::err_drv_no_input_files);
+    return;
+  }
+
+  // Reject -Z* at the top level, these options should never have been exposed
+  // by gcc.
+  if (Arg *A = Args.getLastArg(options::OPT_Z_Joined))
+    Diag(clang::diag::err_drv_use_of_Z_option) << A->getAsString(Args);
+
+  // Diagnose misuse of /Fo.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_Fo)) {
+    StringRef V = A->getValue();
+    if (Inputs.size() > 1 && !V.empty() &&
+        !llvm::sys::path::is_separator(V.back())) {
+      // Check whether /Fo tries to name an output file for multiple inputs.
+      Diag(clang::diag::err_drv_out_file_argument_with_multiple_sources)
+          << A->getSpelling() << V;
+      Args.eraseArg(options::OPT__SLASH_Fo);
+    }
+  }
+
+  // Diagnose misuse of /Fa.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_Fa)) {
+    StringRef V = A->getValue();
+    if (Inputs.size() > 1 && !V.empty() &&
+        !llvm::sys::path::is_separator(V.back())) {
+      // Check whether /Fa tries to name an asm file for multiple inputs.
+      Diag(clang::diag::err_drv_out_file_argument_with_multiple_sources)
+          << A->getSpelling() << V;
+      Args.eraseArg(options::OPT__SLASH_Fa);
+    }
+  }
+
+  // Diagnose misuse of /o.
+  if (Arg *A = Args.getLastArg(options::OPT__SLASH_o)) {
+    if (A->getValue()[0] == '\0') {
+      // It has to have a value.
+      Diag(clang::diag::err_drv_missing_argument) << A->getSpelling() << 1;
+      Args.eraseArg(options::OPT__SLASH_o);
+    }
+  }
+
+  // Ignore /Yc/Yu if both /Yc and /Yu passed but with different filenames.
+  Arg *YcArg = Args.getLastArg(options::OPT__SLASH_Yc);
+  Arg *YuArg = Args.getLastArg(options::OPT__SLASH_Yu);
+  if (YcArg && YuArg && strcmp(YcArg->getValue(), YuArg->getValue()) != 0) {
+    Diag(clang::diag::warn_drv_ycyu_different_arg_clang_cl);
+    Args.eraseArg(options::OPT__SLASH_Yc);
+    Args.eraseArg(options::OPT__SLASH_Yu);
+    YcArg = YuArg = nullptr;
+  }
+  if (YcArg && Inputs.size() > 1) {
+    Diag(clang::diag::warn_drv_yc_multiple_inputs_clang_cl);
+    Args.eraseArg(options::OPT__SLASH_Yc);
+    YcArg = nullptr;
+  }
+
+  // Builder to be used to build offloading actions.
+  OffloadingActionBuilder OffloadBuilder(C, Args, Inputs);
+
+  // Construct the actions to perform.
+  HeaderModulePrecompileJobAction *HeaderModuleAction = nullptr;
+  ActionList LinkerInputs;
+
+  handleArguments(C, Args, Inputs, Actions, YcArg, YuArg);
+
   for (auto &I : Inputs) {
     types::ID InputType = I.first;
     const Arg *InputArg = I.second;
 
     llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PL;
     types::getCompilationPhases(*this, Args, InputType, PL);
     if (PL.empty())
       continue;