[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 ↗

(On Diff #214545)

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 ↗

(On Diff #214545)

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 ↗

(On Diff #214545)

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!

include/clang/Driver/Driver.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"
 

 
   // 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) const;
+
   // Before executing jobs, sets up response files for commands that need them.
   void setUpResponseFiles(Compilation &C, Command &Cmd);
 

lib/Driver/Driver.cpp

 };
 } // anonymous namespace.
 
+void Driver::handleArguments(Compilation &C, DerivedArgList &Args,
+                             const InputList &Inputs,
+                             ActionList &Actions) const {
+
+  // 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;
+  }
+
+  Arg *FinalPhaseArg;
+  phases::ID FinalPhase = getFinalPhase(Args, &FinalPhaseArg);
+
+  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;
+  }
+
+  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");

     }
   }
 
-  // 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;
-  }
+  handleArguments(C, Args, Inputs, Actions);
 
   // Builder to be used to build offloading actions.
   OffloadingActionBuilder OffloadBuilder(C, Args, Inputs);

   HeaderModulePrecompileJobAction *HeaderModuleAction = nullptr;
   ActionList LinkerInputs;
 
-  {
-    Arg *FinalPhaseArg;
-    phases::ID FinalPhase = getFinalPhase(Args, &FinalPhaseArg);
-
-    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;
-    }
-
-    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);
-    }
-  }
-
   for (auto &I : Inputs) {
     types::ID InputType = I.first;
     const Arg *InputArg = I.second;