More fixes.

Author: David Ungar

include/swift/Driver/DependencyGraph.h

@@ -121,12 +121,6 @@ class DependencyGraphImpl {
   llvm::StringMap<std::pair<std::vector<DependencyEntryTy>, DependencyMaskTy>> Dependencies;
 
   /// The set of marked nodes.
-  /// "Marked" means that everything provided by this node (i.e. Job) is
-  /// dirty. Thus any file using any of these provides must be recompiled.
-  /// (Only non-private entities are output as provides.)
-  /// In other words, this Job "cascades"; the need to recompile it causes other
-  /// recompilations. It is possible that the current code marks things that do
-  /// not need to be marked. Nothing would break if that were the case.
 
   llvm::SmallPtrSet<const void *, 16> Marked;
 
@@ -263,7 +257,7 @@ class DependencyGraph : public DependencyGraphImpl {
   /// Marks \p node and all nodes that depend on \p node, and places any nodes
   /// that get transitively marked into \p visited.
   ///
-  /// Nodes that have been previously marked are not included in \p newlyMarked,
+  /// Nodes that have been previously marked are not included in \p visited,
   /// nor are their successors traversed, <em>even if their "provides" set has
   /// been updated since it was marked.</em> (However, nodes that depend on the
   /// given \p node are always traversed.)
@@ -279,9 +273,8 @@ class DependencyGraph : public DependencyGraphImpl {
   /// visited node, add it to visited, and mark it if it cascades. The start
   /// node is NOT added to visited.
   ///
-  /// Do not confused "Marked" with "Visited".
-  /// "Marked" does NOT influence the traversal. The traversal routines use
-  /// "Visited" to avoid endless "recursion".
+  /// The traversal routines use
+  /// \p visited to avoid endless recursion.
   template <unsigned N>
   void markTransitive(SmallVector<T, N> &visited, T node,
                       MarkTracer *tracer = nullptr) {

include/swift/Driver/Job.h

@@ -231,19 +231,19 @@ class Job {
 public:
   enum class Condition {
     // There was no information about the previous build (i.e., an input map),
-    // or the map marked this Job as dirty
-    // but the input didn't change.
+    // or the map marked this Job as dirty or needing a cascading build.
     // Be maximally conservative with dependencies.
     Always,
-    // The input changed, or this job was non-cascading or [it was not dirty but
-    // primary output was missing].
-    // In other words, this job was scheduled as non-cascading in the last build
+    // The input changed, or this job was scheduled as non-cascading in the last
+    // build
     // but didn't get to run.
+    // The scheduled-but-didn't-run condition is detected when the job was not
+    // dirty but its primary output was missing.
     RunWithoutCascading,
     // The best case: input didn't change, output exists.
     // Only run if it depends on some other thing that changed.
     CheckDependencies,
-    // Pessimal: Run no matter what (but may or may not cascade).
+    // Run no matter what (but may or may not cascade).
     NewlyAdded
   };
 

lib/Driver/Compilation.cpp

@@ -209,7 +209,7 @@ namespace driver {
     /// Jobs in the initial set with Condition::Always, and having an existing
     /// .swiftdeps files.
     /// Set by scheduleInitialJobs and used only by scheduleAdditionalJobs.
-    SmallVector<const Job *, 16> InitialOutOfDateCommands;
+    SmallVector<const Job *, 16> InitialCascadingCommands;
 
     /// Dependency graph for deciding which jobs are dirty (need running)
     /// or clean (can be skipped).
@@ -407,6 +407,13 @@ namespace driver {
         // If we have a dependency file /and/ the frontend task exited normally,
         // we can be discerning about what downstream files to rebuild.
         if (ReturnCode == EXIT_SUCCESS || ReturnCode == EXIT_FAILURE) {
+          // "Marked" means that everything provided by this node (i.e. Job) is
+          // dirty. Thus any file using any of these provides must be
+          // recompiled. (Only non-private entities are output as provides.) In
+          // other words, this Job "cascades"; the need to recompile it causes
+          // other recompilations. It is possible that the current code marks
+          // things that do not need to be marked. Unecessary compilation would
+          // result if that were the case.
           bool wasCascading = DepGraph.isMarked(FinishedCmd);
 
           switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
@@ -717,14 +724,14 @@ namespace driver {
         case Job::Condition::Always:
           if (Comp.getIncrementalBuildEnabled() && !DependenciesFile.empty()) {
             // Ensure dependents will get recompiled.
-            InitialOutOfDateCommands.push_back(Cmd);
+            InitialCascadingCommands.push_back(Cmd);
             // Mark this job as cascading.
             //
             // It would probably be safe and simpler to markTransitive on the
             // start nodes in the "Always" condition from the start instead of
             // using markIntransitive and having later functions call
-            // markTransitive. That way markInstransitive would be an
-            // implementation detail.
+            // markTransitive. That way markIntransitive would be an
+            // implementation detail of DependencyGraph.
             DepGraph.markIntransitive(Cmd);
           }
           LLVM_FALLTHROUGH;
@@ -749,7 +756,7 @@ namespace driver {
         // We scheduled all of the files that have actually changed. Now add the
         // files that haven't changed, so that they'll get built in parallel if
         // possible and after the first set of files if it's not.
-        for (auto *Cmd : InitialOutOfDateCommands) {
+        for (auto *Cmd : InitialCascadingCommands) {
           DepGraph.markTransitive(AdditionalOutOfDateCommands, Cmd,
                                   IncrementalTracer);
         }