Add some clarifying comments to CppCompilationContext.
--
MOS_MIGRATED_REVID=95738396
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 e9b0ce5..2ba49d7 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 @@
* <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 @@
* <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 37ec191..ed83b04 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 @@
* <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 @@
* 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 fb2d57d..1728c299 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 @@
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 @@
* (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 @@
* "-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 @@
* "-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 @@
}
/**
- * 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 @@
return builder.build();
}
-
+
/**
* @return modules maps from direct dependencies.
*/
@@ -267,7 +270,7 @@
}
return builder.build();
}
-
+
/**
* @return modules maps in the transitive closure that are not from direct dependencies.
*/
@@ -290,7 +293,7 @@
}
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 @@
}
return builder.build();
}
-
+
/**
* @return all declared headers of the current module if the current target
* is compiled as a module.
@@ -314,10 +317,10 @@
}
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 @@
}
return builder.build();
}
-
+
/**
* @return all header modules in the transitive closure of {@code getTopLevelHeaderModules()}.
*/
@@ -337,7 +340,7 @@
}
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 @@
@Override
public int hashCode() {
- return Objects.hash(headerModule, picHeaderModule, commandLineContext, depsContexts);
+ return Objects.hash(
+ headerModule, picHeaderModule, commandLineContext, depsContexts, cppModuleMap);
}
/**
@@ -397,8 +401,8 @@
* 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 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 @@
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 @@
* 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 @@
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 @@
&& 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 @@
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 @@
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 @@
// 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 @@
transitiveModuleMapsForHeaderModules.add(otherContext.getCppModuleMap().getArtifact());
}
}
-
+
defines.addAll(otherContext.getDefines());
return this;
}
@@ -874,7 +881,7 @@
this.cppModuleMap = cppModuleMap;
return this;
}
-
+
/**
* Sets the C++ header module in non-pic mode.
*/
@@ -890,7 +897,7 @@
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 15d7010..7702a17 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 @@
* 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
