Make Driver::BuildJobsForAction return an InputInfo, instead of using an outparam.

[jlebar]

Summary

Explicit is better than implicit.

[echristo]

One inline comment. Basic idea is that having the a "result" also be an "inputinfo" is confusing. Not that I have a better name for InputInfo, but if you come up with one...

include/clang/Driver/Driver.h

383 ↗

(On Diff #44376)

Update the comment?

[jlebar]

In D16013#322649, @echristo wrote:

having the a "result" also be an "inputinfo" is confusing. Not that I have a better name for InputInfo, but if you come up with one...

Agree, I will meditate on this (for a separate patch).

[jlebar]

Address echristo's review.

[echristo]

Thanks!

-eric

[jlebar]

In D16013#322649, @echristo wrote:

having the a "result" also be an "inputinfo" is confusing. Not that I have a better name for InputInfo, but if you come up with one...

Agree, I will meditate on this (for a separate patch).

cfe/trunk/include/clang/Driver/Driver.h

public:
   /// ConstructAction - Construct the appropriate action to do for
   /// \p Phase on the \p Input, taking in to account arguments
   /// like -fsyntax-only or --analyze.
   Action *ConstructPhaseAction(Compilation &C, const ToolChain &TC,
                                const llvm::opt::ArgList &Args, phases::ID Phase,
                                Action *Input) const;
 
   /// BuildJobsForAction - Construct the jobs to perform for the
-  /// action \p A.
-  void BuildJobsForAction(Compilation &C,
-                          const Action *A,
-                          const ToolChain *TC,
-                          const char *BoundArch,
-                          bool AtTopLevel,
-                          bool MultipleArchs,
-                          const char *LinkingOutput,
-                          InputInfo &Result) const;
+  /// action \p A and return an InputInfo for the result of running \p A.
+  InputInfo BuildJobsForAction(Compilation &C, const Action *A,
+                               const ToolChain *TC, const char *BoundArch,
+                               bool AtTopLevel, bool MultipleArchs,
+                               const char *LinkingOutput) const;
 
   /// Returns the default name for linked images (e.g., "a.out").
   const char *getDefaultImageName() const;
 
   /// GetNamedOutputPath - Return the name to use for the output of
   /// the action \p JA. The result is appended to the compilation's
   /// list of temporary or result files, as appropriate.
   ///

cfe/trunk/lib/Driver/Driver.cpp

for (Action *A : C.getActions()) {
     const char *LinkingOutput = nullptr;
     if (isa<LipoJobAction>(A)) {
       if (FinalOutput)
         LinkingOutput = FinalOutput->getValue();
       else
         LinkingOutput = getDefaultImageName();
     }
 
-    InputInfo II;
     BuildJobsForAction(C, A, &C.getDefaultToolChain(),
                        /*BoundArch*/ nullptr,
                        /*AtTopLevel*/ true,
                        /*MultipleArchs*/ ArchNames.size() > 1,
-                       /*LinkingOutput*/ LinkingOutput, II);
+                       /*LinkingOutput*/ LinkingOutput);
   }
 
   // If the user passed -Qunused-arguments or there were errors, don't warn
   // about any unused arguments.
   if (Diags.hasErrorOccurred() ||
       C.getArgs().hasArg(options::OPT_Qunused_arguments))
     return;
 
if (Inputs->size() == 1 && isa<PreprocessJobAction>(*Inputs->begin()) &&
       !C.getArgs().hasArg(options::OPT_traditional_cpp) && !SaveTemps &&
       !C.getArgs().hasArg(options::OPT_rewrite_objc) &&
       ToolForJob->hasIntegratedCPP())
     Inputs = &(*Inputs)[0]->getInputs();
 
   return ToolForJob;
 }
 
-void Driver::BuildJobsForAction(Compilation &C, const Action *A,
+InputInfo Driver::BuildJobsForAction(Compilation &C, const Action *A,
                                      const ToolChain *TC, const char *BoundArch,
                                      bool AtTopLevel, bool MultipleArchs,
-                                const char *LinkingOutput,
-                                InputInfo &Result) const {
+                                     const char *LinkingOutput) const {
   llvm::PrettyStackTraceString CrashInfo("Building compilation jobs");
 
   InputInfoList CudaDeviceInputInfos;
   if (const CudaHostAction *CHA = dyn_cast<CudaHostAction>(A)) {
-    InputInfo II;
     // Append outputs of device jobs to the input list.
     for (const Action *DA : CHA->getDeviceActions()) {
+      CudaDeviceInputInfos.push_back(
           BuildJobsForAction(C, DA, TC, nullptr, AtTopLevel,
-                         /*MultipleArchs*/ false, LinkingOutput, II);
-      CudaDeviceInputInfos.push_back(II);
+                             /*MultipleArchs*/ false, LinkingOutput));
     }
     // Override current action with a real host compile action and continue
     // processing it.
     A = *CHA->begin();
   }
 
   if (const InputAction *IA = dyn_cast<InputAction>(A)) {
     // FIXME: It would be nice to not claim this here; maybe the old scheme of
     // just using Args was better?
     const Arg &Input = IA->getInputArg();
     Input.claim();
     if (Input.getOption().matches(options::OPT_INPUT)) {
       const char *Name = Input.getValue();
-      Result = InputInfo(Name, A->getType(), Name);
-    } else {
-      Result = InputInfo(&Input, A->getType(), "");
+      return InputInfo(Name, A->getType(), Name);
     }
-    return;
+    return InputInfo(&Input, A->getType(), "");
   }
 
   if (const BindArchAction *BAA = dyn_cast<BindArchAction>(A)) {
     const ToolChain *TC;
     const char *ArchName = BAA->getArchName();
 
     if (ArchName)
       TC = &getToolChain(
           C.getArgs(),
           computeTargetTriple(DefaultTargetTriple, C.getArgs(), ArchName));
     else
       TC = &C.getDefaultToolChain();
 
-    BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel,
-                       MultipleArchs, LinkingOutput, Result);
-    return;
+    return BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel,
+                              MultipleArchs, LinkingOutput);
   }
 
   if (const CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) {
     // Initial processing of CudaDeviceAction carries host params.
     // Call BuildJobsForAction() again, now with correct device parameters.
     assert(CDA->getGpuArchName() && "No GPU name in device action.");
-    BuildJobsForAction(C, *CDA->begin(), C.getCudaDeviceToolChain(),
+    return BuildJobsForAction(C, *CDA->begin(), C.getCudaDeviceToolChain(),
                               CDA->getGpuArchName(), CDA->isAtTopLevel(),
-                       /*MultipleArchs*/ true, LinkingOutput, Result);
-    return;
+                              /*MultipleArchs*/ true, LinkingOutput);
   }
 
   const ActionList *Inputs = &A->getInputs();
 
   const JobAction *JA = cast<JobAction>(A);
   const CudaHostAction *CollapsedCHA = nullptr;
   const Tool *T =
       selectToolForJob(C, isSaveTempsEnabled(), TC, JA, Inputs, CollapsedCHA);
   if (!T)
-    return;
+    return InputInfo();
 
   // If we've collapsed action list that contained CudaHostAction we
   // need to build jobs for device-side inputs it may have held.
   if (CollapsedCHA) {
-    InputInfo II;
     for (const Action *DA : CollapsedCHA->getDeviceActions()) {
+      CudaDeviceInputInfos.push_back(
           BuildJobsForAction(C, DA, TC, "", AtTopLevel,
-                         /*MultipleArchs*/ false, LinkingOutput, II);
-      CudaDeviceInputInfos.push_back(II);
+                             /*MultipleArchs*/ false, LinkingOutput));
     }
   }
 
   // Only use pipes when there is exactly one input.
   InputInfoList InputInfos;
   for (const Action *Input : *Inputs) {
     // Treat dsymutil and verify sub-jobs as being at the top-level too, they
     // shouldn't get temporary output names.
     // FIXME: Clean this up.
-    bool SubJobAtTopLevel = false;
-    if (AtTopLevel && (isa<DsymutilJobAction>(A) || isa<VerifyJobAction>(A)))
-      SubJobAtTopLevel = true;
-
-    InputInfo II;
-    BuildJobsForAction(C, Input, TC, BoundArch, SubJobAtTopLevel, MultipleArchs,
-                       LinkingOutput, II);
-    InputInfos.push_back(II);
+    bool SubJobAtTopLevel =
+        AtTopLevel && (isa<DsymutilJobAction>(A) || isa<VerifyJobAction>(A));
+    InputInfos.push_back(BuildJobsForAction(C, Input, TC, BoundArch,
+                                            SubJobAtTopLevel, MultipleArchs,
+                                            LinkingOutput));
   }
 
   // Always use the first input as the base input.
   const char *BaseInput = InputInfos[0].getBaseInput();
 
   // ... except dsymutil actions, which use their actual input as the base
   // input.
   if (JA->getType() == types::TY_dSYM)
     BaseInput = InputInfos[0].getFilename();
 
   // Append outputs of cuda device jobs to the input list
   if (CudaDeviceInputInfos.size())
     InputInfos.append(CudaDeviceInputInfos.begin(), CudaDeviceInputInfos.end());
 
   // Determine the place to write output to, if any.
+  InputInfo Result;
   if (JA->getType() == types::TY_Nothing)
     Result = InputInfo(A->getType(), BaseInput);
   else
     Result = InputInfo(GetNamedOutputPath(C, *JA, BaseInput, BoundArch,
                                           AtTopLevel, MultipleArchs),
                        A->getType(), BaseInput);
 
   if (CCCPrintBindings && !CCGenDiagnostics) {
     llvm::errs() << "# \"" << T->getToolChain().getTripleString() << '"'
                  << " - \"" << T->getName() << "\", inputs: [";
     for (unsigned i = 0, e = InputInfos.size(); i != e; ++i) {
       llvm::errs() << InputInfos[i].getAsString();
       if (i + 1 != e)
         llvm::errs() << ", ";
     }
     llvm::errs() << "], output: " << Result.getAsString() << "\n";
   } else {
     T->ConstructJob(C, *JA, Result, InputInfos,
                     C.getArgsForToolChain(TC, BoundArch), LinkingOutput);
   }
+  return Result;
 }
 
 const char *Driver::getDefaultImageName() const {
   llvm::Triple Target(llvm::Triple::normalize(DefaultTargetTriple));
   return Target.isOSWindows() ? "a.exe" : "a.out";
 }
 
 /// \brief Create output filename based on ArgValue, which could either be a