Add some clarifying comments to CppCompilationContext.

--
MOS_MIGRATED_REVID=95738396

4 files changed, 67 insertions, 56 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
index e9b0ce5f65..2ba49d79d9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoCollection.java
@@ -31,7 +31,7 @@ import javax.annotation.Nullable;
  * <ul>
  * <li>The associated Target (which will usually be a Rule)
  * <li>Its own configuration (the configured target does not have access to other configurations,
- * e.g. the host configuration, though)
+ * e.g. the host configuration)
  * <li>The transitive info providers and labels of its direct dependencies.
  * </ul>
  *
@@ -70,7 +70,7 @@ import javax.annotation.Nullable;
  * <li>Serialize / deserialize individual configured targets at will, making it possible for
  * example to swap out part of the analysis state if there is memory pressure or to move them in
  * persistent storage so that the state can be reconstructed at a different time or in a
- * different process. The stretch goal is to eventually facilitate cross-uses caching of this
+ * different process. The stretch goal is to eventually facilitate cross-user caching of this
  * information.
  * </ul>
  *
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProvider.java
index 37ec1910cb..ed83b04b50 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/TransitiveInfoProvider.java
@@ -29,11 +29,11 @@ package com.google.devtools.build.lib.analysis;
  * <li>Overloading a method name multiple times is forbidden.</li>
  * <li>The return type of a method must satisfy one of the following conditions:
  * <ul>
- * <li>It must be from the set of {String, Integer, int, Boolean, bool, Label, PathFragment,
+ *  <li>It must be from the set of {String, Integer, int, Boolean, bool, Label, PathFragment,
  * Artifact}, OR</li>
- * <li>it must be an ImmutableList/List/Collection/Iterable of T, where T is either
+ *  <li>it must be an ImmutableList/List/Collection/Iterable of T, where T is either
  * one of the types above with a default serializer or T implements ValueSerializer), OR</li>
- * <li>it must be serializable (TBD)</li>
+ *  <li>it must be serializable (TBD)</li>
  * </ul>
  * <li>If the method takes arguments, it must declare a custom serializer (TBD).</li>
  * </ul>
@@ -49,7 +49,8 @@ package com.google.devtools.build.lib.analysis;
  * being O(n^2): in a long dependency chain, if every target adds one single artifact, storing the
  * transitive closures of every rule would take 1+2+3+...+n-1+n = O(n^2) memory.
  *
- * <p>In order to avoid this, we introduce the concept of nested sets. A nested set is an immutable
+ * <p>In order to avoid this, we introduce the concept of nested sets, {@link com.google.devtools
+ * .build.lib.collect.nestedset.NestedSet}. A nested set is an immutable
  * data structure that can contain direct members and other nested sets (recursively). Nested sets
  * are iterable and can be flattened into ordered sets, where the order depends on which
  * implementation of NestedSet you pick.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
index fb2d57d898..1728c2998f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompilationContext.java
@@ -52,6 +52,8 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
   private final CppModuleMap cppModuleMap;
   private final Artifact headerModule;
   private final Artifact picHeaderModule;
+
+  // Derived from depsContexts; no need to consider it for equals/hashCode.
   private final ImmutableSet<Artifact> compilationPrerequisites;
 
   private CppCompilationContext(CommandLineContext commandLineContext,
@@ -107,7 +109,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
    * (possibly empty but never null). This includes the include dirs from the
    * transitive deps closure of the target. This list does not contain
    * duplicates. All fragments are either absolute or relative to the exec root
-   * (see {@link BuildConfiguration#getExecRoot}).
+   * (see {@link com.google.devtools.build.lib.analysis.BlazeDirectories#getExecRoot}).
    */
   public ImmutableList<PathFragment> getIncludeDirs() {
     return commandLineContext.includeDirs;
@@ -118,7 +120,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
    * "-iquote" (possibly empty but never null). This includes the include dirs
    * from the transitive deps closure of the target. This list does not contain
    * duplicates. All fragments are either absolute or relative to the exec root
-   * (see {@link BuildConfiguration#getExecRoot}).
+   * (see {@link com.google.devtools.build.lib.analysis.BlazeDirectories#getExecRoot}).
    */
   public ImmutableList<PathFragment> getQuoteIncludeDirs() {
     return commandLineContext.quoteIncludeDirs;
@@ -129,7 +131,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
    * "-isystem" (possibly empty but never null). This includes the include dirs
    * from the transitive deps closure of the target. This list does not contain
    * duplicates. All fragments are either absolute or relative to the exec root
-   * (see {@link BuildConfiguration#getExecRoot}).
+   * (see {@link com.google.devtools.build.lib.analysis.BlazeDirectories#getExecRoot}).
    */
   public ImmutableList<PathFragment> getSystemIncludeDirs() {
     return commandLineContext.systemIncludeDirs;
@@ -215,9 +217,10 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
   }
 
   /**
-   * Returns the immutable pairs of (header file, pregrepped header file).
+   * Returns the immutable pairs of (header file, pregrepped header file).  The value artifacts
+   * (pregrepped header file) are generated by {@link ExtractInclusionAction}.
    */
-  public NestedSet<Pair<Artifact, Artifact>> getPregreppedHeaders() {
+  NestedSet<Pair<Artifact, Artifact>> getPregreppedHeaders() {
     if (depsContexts.isEmpty()) {
       return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
     }
@@ -256,7 +259,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
 
     return builder.build();
   }
-  
+
   /**
    * @return modules maps from direct dependencies.
    */
@@ -267,7 +270,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * @return modules maps in the transitive closure that are not from direct dependencies.
    */
@@ -290,7 +293,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * @return all headers whose transitive closure of includes needs to be
    * available when compiling anything in the current target.
@@ -302,7 +305,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * @return all declared headers of the current module if the current target
    * is compiled as a module.
@@ -314,10 +317,10 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * @return all header modules in our transitive closure that are not in the transitive closure
-   * of another header module in our transitive closure.  
+   * of another header module in our transitive closure.
    */
   protected NestedSet<Artifact> getTopLevelHeaderModules() {
     NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
@@ -326,7 +329,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * @return all header modules in the transitive closure of {@code getTopLevelHeaderModules()}.
    */
@@ -337,7 +340,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     }
     return builder.build();
   }
-  
+
   /**
    * Returns the set of defines needed to compile this target (possibly empty
    * but never null). This includes definitions from the transitive deps closure
@@ -365,7 +368,8 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
 
   @Override
   public int hashCode() {
-    return Objects.hash(headerModule, picHeaderModule, commandLineContext, depsContexts);
+    return Objects.hash(
+        headerModule, picHeaderModule, commandLineContext, depsContexts, cppModuleMap);
   }
 
   /**
@@ -397,8 +401,8 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
    * Returns the context for a LIPO compile action. This uses the include dirs
    * and defines of the library, but the declared inclusion dirs/srcs from both
    * the library and the owner binary.
-
-   * TODO(bazel-team): this might make every LIPO target have an unnecessary large set of
+   *
+   * <p>TODO(bazel-team): this might make every LIPO target have an unnecessary large set of
    * inclusion dirs/srcs. The correct behavior would be to merge only the contexts
    * of actual referred targets (as listed in .imports file).
    *
@@ -431,14 +435,14 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
   public CppModuleMap getCppModuleMap() {
     return cppModuleMap;
   }
-  
+
   /**
    * @return the non-pic C++ header module of the owner.
    */
   private Artifact getHeaderModule() {
     return headerModule;
   }
-  
+
   /**
    * @return the pic C++ header module of the owner.
    */
@@ -498,31 +502,36 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
     private final NestedSet<PathFragment> declaredIncludeDirs;
     private final NestedSet<PathFragment> declaredIncludeWarnDirs;
     private final NestedSet<Artifact> declaredIncludeSrcs;
-    private final NestedSet<Pair<Artifact, Artifact>> pregreppedHdrs;
-    
+    /**
+     * Module maps from direct dependencies.
+     */
+    private final NestedSet<Artifact> directModuleMaps;
+
     /**
      * All declared headers of the current module, if compiled as a header module.
      */
     private final NestedSet<Artifact> headerModuleSrcs;
-    
+
+    /**
+     * All header modules in the transitive closure of {@code topLevelHeaderModules}.
+     */
+    private final NestedSet<Artifact> impliedHeaderModules;
+
+    private final NestedSet<Pair<Artifact, Artifact>> pregreppedHdrs;
+
     /**
      * All header modules in our transitive closure that are not in the transitive closure of
      * another header module in our transitive closure.
      */
     private final NestedSet<Artifact> topLevelHeaderModules;
-    
-    /**
-     * All header modules in the transitive closure of {@code topLevelHeaderModules}.
-     */
-    private final NestedSet<Artifact> impliedHeaderModules;
-    
+
     /**
      * Headers whose transitive closure of includes needs to be available when compiling the current
      * target. For every target that the current target depends on transitively and that is built as
      * header module, contains all headers that are part of its header module.
      */
     private final NestedSet<Artifact> transitiveHeaderModuleSrcs;
-    
+
     /**
      * The module maps from all targets the current target depends on transitively.
      */
@@ -532,12 +541,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
      * Module maps from targets in the transitive closure that are not from direct dependencies.
      */
     private final NestedSet<Artifact> transitiveModuleMapsForHeaderModules;
-    
-    /**
-     * Module maps from direct dependencies.
-     */
-    private final NestedSet<Artifact> directModuleMaps;
-    
+
     DepsContext(Artifact compilationPrerequisiteStampFile,
         NestedSet<PathFragment> declaredIncludeDirs,
         NestedSet<PathFragment> declaredIncludeWarnDirs,
@@ -554,14 +558,14 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       this.declaredIncludeDirs = declaredIncludeDirs;
       this.declaredIncludeWarnDirs = declaredIncludeWarnDirs;
       this.declaredIncludeSrcs = declaredIncludeSrcs;
-      this.pregreppedHdrs = pregreppedHdrs;
+      this.directModuleMaps = directModuleMaps;
       this.headerModuleSrcs = headerModuleSrcs;
-      this.topLevelHeaderModules = topLevelHeaderModules;
       this.impliedHeaderModules = impliedHeaderModules;
-      this.transitiveHeaderModuleSrcs = transitiveHeaderModuleSrcs;
+      this.pregreppedHdrs = pregreppedHdrs;
+      this.topLevelHeaderModules = topLevelHeaderModules;
       this.transitiveModuleMaps = transitiveModuleMaps;
       this.transitiveModuleMapsForHeaderModules = transitiveModuleMapsForHeaderModules;
-      this.directModuleMaps = directModuleMaps;
+      this.transitiveHeaderModuleSrcs = transitiveHeaderModuleSrcs;
     }
 
     @Override
@@ -578,29 +582,32 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
           && Objects.equals(declaredIncludeDirs, other.declaredIncludeDirs)
           && Objects.equals(declaredIncludeWarnDirs, other.declaredIncludeWarnDirs)
           && Objects.equals(declaredIncludeSrcs, other.declaredIncludeSrcs)
+          && Objects.equals(directModuleMaps, other.directModuleMaps)
           && Objects.equals(headerModuleSrcs, other.headerModuleSrcs)
-          && Objects.equals(topLevelHeaderModules, other.topLevelHeaderModules)
           && Objects.equals(impliedHeaderModules, other.impliedHeaderModules)
+          // TODO(bazel-team): add pregreppedHdrs?
+          && Objects.equals(topLevelHeaderModules, other.topLevelHeaderModules)
           && Objects.equals(transitiveHeaderModuleSrcs, other.transitiveHeaderModuleSrcs)
           && Objects.equals(transitiveModuleMaps, other.transitiveModuleMaps)
           && Objects.equals(
-              transitiveModuleMapsForHeaderModules, other.transitiveModuleMapsForHeaderModules)
-          && Objects.equals(directModuleMaps, other.directModuleMaps);
+              transitiveModuleMapsForHeaderModules, other.transitiveModuleMapsForHeaderModules);
     }
 
     @Override
     public int hashCode() {
-      return Objects.hash(compilationPrerequisiteStampFile,
+      return Objects.hash(
+          compilationPrerequisiteStampFile,
           declaredIncludeDirs,
           declaredIncludeWarnDirs,
           declaredIncludeSrcs,
+          directModuleMaps,
           headerModuleSrcs,
-          topLevelHeaderModules,
           impliedHeaderModules,
+          // pregreppedHdrs ?
           transitiveHeaderModuleSrcs,
           transitiveModuleMaps,
           transitiveModuleMapsForHeaderModules,
-          directModuleMaps);
+          topLevelHeaderModules);
     }
   }
 
@@ -684,7 +691,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       declaredIncludeWarnDirs.addTransitive(otherContext.getDeclaredIncludeWarnDirs());
       declaredIncludeSrcs.addTransitive(otherContext.getDeclaredIncludeSrcs());
       pregreppedHdrs.addTransitive(otherContext.getPregreppedHeaders());
-      
+
       NestedSet<Artifact> othersTransitiveModuleMaps = otherContext.getTransitiveModuleMaps();
       NestedSet<Artifact> othersDirectModuleMaps = otherContext.getDirectModuleMaps();
       NestedSet<Artifact> othersTopLevelHeaderModules = otherContext.getTopLevelHeaderModules();
@@ -699,7 +706,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       transitiveHeaderModuleSrcs.addTransitive(otherContext.getTransitiveHeaderModuleSrcs());
       impliedHeaderModules.addTransitive(otherContext.getImpliedHeaderModules());
       topLevelHeaderModules.addTransitive(othersTopLevelHeaderModules);
-      
+
       // All module maps of direct dependencies are inputs to the current compile independently of
       // the build type.
       if (otherContext.getCppModuleMap() != null) {
@@ -719,7 +726,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
         // All targets transitively depending on us will need to have the full transitive #include
         // closure of the headers in that module available.
         transitiveHeaderModuleSrcs.addTransitive(otherContext.getHeaderModuleSrcs());
-        
+
         // To use a header module we need the full transitive closure of module maps of the
         // target that provides the header module, including its own module map.
         transitiveModuleMapsForHeaderModules.addTransitive(othersTransitiveModuleMaps);
@@ -728,7 +735,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
           transitiveModuleMapsForHeaderModules.add(otherContext.getCppModuleMap().getArtifact());
         }
       }
-      
+
       defines.addAll(otherContext.getDefines());
       return this;
     }
@@ -874,7 +881,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       this.cppModuleMap = cppModuleMap;
       return this;
     }
-    
+
     /**
      * Sets the C++ header module in non-pic mode.
      */
@@ -890,7 +897,7 @@ public final class CppCompilationContext implements TransitiveInfoProvider {
       this.picHeaderModule = picHeaderModule;
       return this;
     }
-    
+
     /**
      * Builds the {@link CppCompilationContext}.
      */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ExtractInclusionAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ExtractInclusionAction.java
index 15d7010f7c..7702a17816 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/ExtractInclusionAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/ExtractInclusionAction.java
@@ -29,6 +29,9 @@ import java.io.IOException;
  * An action which greps for includes over a given .cc or .h file.
  * This is a part of the work required for C++ include scanning.
  *
+ * <p>For generated files, it is advantageous to do this remotely, to avoid having to download
+ * the generated file.
+ *
  * <p>Note that this may run grep-includes over-optimistically, where we previously
  * had not. For example, consider a cc_library of generated headers. If another
  * library depends on it, and only references one of the headers, the other