Make distinct `direct_public_headers` and `direct_private_headers` fields on `CcCompilationContext` that are accessible from Starlark.
The `direct_headers` field still remains as the union of these two.
RELNOTES: None.
PiperOrigin-RevId: 313206379
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java
index 5c62cec..e48af69 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java
@@ -167,7 +167,21 @@
@Override
public StarlarkList<Artifact> getStarlarkDirectModularHeaders() {
- return StarlarkList.immutableCopyOf(getDirectHdrs());
+ return StarlarkList.immutableCopyOf(
+ ImmutableList.<Artifact>builder()
+ .addAll(getDirectPublicHdrs())
+ .addAll(getDirectPrivateHdrs())
+ .build());
+ }
+
+ @Override
+ public StarlarkList<Artifact> getStarlarkDirectPublicHeaders() {
+ return StarlarkList.immutableCopyOf(getDirectPublicHdrs());
+ }
+
+ @Override
+ public StarlarkList<Artifact> getStarlarkDirectPrivateHeaders() {
+ return StarlarkList.immutableCopyOf(getDirectPrivateHdrs());
}
@Override
@@ -305,9 +319,14 @@
return headerInfo.textualHeaders;
}
- /** Returns headers given as textual_hdrs in this target. */
- public ImmutableList<Artifact> getDirectHdrs() {
- return headerInfo.modularHeaders;
+ /** Returns public headers (given as {@code hdrs}) in this target. */
+ public ImmutableList<Artifact> getDirectPublicHdrs() {
+ return headerInfo.modularPublicHeaders;
+ }
+
+ /** Returns private headers (given as {@code srcs}) in this target. */
+ public ImmutableList<Artifact> getDirectPrivateHdrs() {
+ return headerInfo.modularPrivateHeaders;
}
public ImmutableList<HeaderInfo> getTransitiveHeaderInfos() {
@@ -362,6 +381,30 @@
}
/**
+ * Passes a list of headers to the include scanning helper for handling, and optionally adds them
+ * to a set that tracks modular headers.
+ *
+ * <p>This is factored out into its own method not only to reduce code duplication below, but also
+ * to improve JIT optimization for this performance-sensitive region.
+ */
+ private static void handleHeadersForIncludeScanning(
+ ImmutableList<Artifact> headers,
+ Map<PathFragment, Artifact> pathToLegalOutputArtifact,
+ ArrayList<Artifact> treeArtifacts,
+ boolean isModule,
+ Set<Artifact> modularHeaders) {
+ // Not using range-based for loops here and below as the additional overhead of the
+ // ImmutableList iterators has shown up in profiles.
+ for (int i = 0; i < headers.size(); i++) {
+ Artifact a = headers.get(i);
+ IncludeScanningHeaderDataHelper.handleArtifact(a, pathToLegalOutputArtifact, treeArtifacts);
+ if (isModule) {
+ modularHeaders.add(a);
+ }
+ }
+ }
+
+ /**
* This method returns null when a required SkyValue is missing and a Skyframe restart is
* required.
*/
@@ -383,23 +426,31 @@
for (int c = 0; c < transitiveHeaderInfoList.size(); c++) {
HeaderInfo transitiveHeaderInfo = transitiveHeaderInfoList.get(c);
boolean isModule = createModularHeaders && transitiveHeaderInfo.getModule(usePic) != null;
- for (int i = 0; i < transitiveHeaderInfo.modularHeaders.size(); i++) {
- Artifact a = transitiveHeaderInfo.modularHeaders.get(i);
- IncludeScanningHeaderDataHelper.handleArtifact(a, pathToLegalOutputArtifact, treeArtifacts);
- if (isModule) {
- modularHeaders.add(a);
- }
- }
- for (int i = 0; i < transitiveHeaderInfo.textualHeaders.size(); i++) {
- Artifact a = transitiveHeaderInfo.textualHeaders.get(i);
- IncludeScanningHeaderDataHelper.handleArtifact(a, pathToLegalOutputArtifact, treeArtifacts);
- }
+ handleHeadersForIncludeScanning(
+ transitiveHeaderInfo.modularPublicHeaders,
+ pathToLegalOutputArtifact,
+ treeArtifacts,
+ isModule,
+ modularHeaders);
+ handleHeadersForIncludeScanning(
+ transitiveHeaderInfo.modularPrivateHeaders,
+ pathToLegalOutputArtifact,
+ treeArtifacts,
+ isModule,
+ modularHeaders);
+ handleHeadersForIncludeScanning(
+ transitiveHeaderInfo.textualHeaders,
+ pathToLegalOutputArtifact,
+ treeArtifacts,
+ /* isModule= */ false,
+ null);
}
if (!IncludeScanningHeaderDataHelper.handleTreeArtifacts(
env, pathToLegalOutputArtifact, treeArtifacts)) {
return null;
}
- removeArtifactsFromSet(modularHeaders, headerInfo.modularHeaders);
+ removeArtifactsFromSet(modularHeaders, headerInfo.modularPublicHeaders);
+ removeArtifactsFromSet(modularHeaders, headerInfo.modularPrivateHeaders);
removeArtifactsFromSet(modularHeaders, headerInfo.textualHeaders);
return new IncludeScanningHeaderData.Builder(
Collections.unmodifiableMap(pathToLegalOutputArtifact),
@@ -429,8 +480,17 @@
Artifact.DerivedArtifact module = transitiveHeaderInfo.getModule(usePic);
// Not using range-based for loops here as often there is exactly one element in this list
// and the amount of garbage created by SingletonImmutableList.iterator() is significant.
- for (int i = 0; i < transitiveHeaderInfo.modularHeaders.size(); i++) {
- Artifact header = transitiveHeaderInfo.modularHeaders.get(i);
+ for (int i = 0; i < transitiveHeaderInfo.modularPublicHeaders.size(); i++) {
+ Artifact header = transitiveHeaderInfo.modularPublicHeaders.get(i);
+ if (includes.contains(header)) {
+ if (module != null) {
+ result.modules.add(module);
+ }
+ result.headers.add(header);
+ }
+ }
+ for (int i = 0; i < transitiveHeaderInfo.modularPrivateHeaders.size(); i++) {
+ Artifact header = transitiveHeaderInfo.modularPrivateHeaders.get(i);
if (includes.contains(header)) {
if (module != null) {
result.modules.add(module);
@@ -494,7 +554,8 @@
*/
protected Set<Artifact> getHeaderModuleSrcs() {
return new ImmutableSet.Builder<Artifact>()
- .addAll(headerInfo.modularHeaders)
+ .addAll(headerInfo.modularPublicHeaders)
+ .addAll(headerInfo.modularPrivateHeaders)
.addAll(headerInfo.textualHeaders)
.build();
}
@@ -853,8 +914,13 @@
return this;
}
- public Builder addModularHdrs(Collection<Artifact> headers) {
- this.headerInfoBuilder.addHeaders(headers);
+ public Builder addModularPublicHdrs(Collection<Artifact> headers) {
+ this.headerInfoBuilder.addPublicHeaders(headers);
+ return this;
+ }
+
+ public Builder addModularPrivateHdrs(Collection<Artifact> headers) {
+ this.headerInfoBuilder.addPrivateHeaders(headers);
return this;
}
@@ -1036,20 +1102,25 @@
private final Artifact.DerivedArtifact picHeaderModule;
- /** All header files that are compiled into this module. */
- private final ImmutableList<Artifact> modularHeaders;
+ /** All public header files that are compiled into this module. */
+ private final ImmutableList<Artifact> modularPublicHeaders;
- /** All header files that are contained in this module. */
+ /** All private header files that are compiled into this module. */
+ private final ImmutableList<Artifact> modularPrivateHeaders;
+
+ /** All textual header files that are contained in this module. */
private final ImmutableList<Artifact> textualHeaders;
HeaderInfo(
Artifact.DerivedArtifact headerModule,
Artifact.DerivedArtifact picHeaderModule,
- ImmutableList<Artifact> modularHeaders,
+ ImmutableList<Artifact> modularPublicHeaders,
+ ImmutableList<Artifact> modularPrivateHeaders,
ImmutableList<Artifact> textualHeaders) {
this.headerModule = headerModule;
this.picHeaderModule = picHeaderModule;
- this.modularHeaders = modularHeaders;
+ this.modularPublicHeaders = modularPublicHeaders;
+ this.modularPrivateHeaders = modularPrivateHeaders;
this.textualHeaders = textualHeaders;
}
@@ -1063,7 +1134,8 @@
public static class Builder {
private Artifact.DerivedArtifact headerModule = null;
private Artifact.DerivedArtifact picHeaderModule = null;
- private final Set<Artifact> modularHeaders = new HashSet<>();
+ private final Set<Artifact> modularPublicHeaders = new HashSet<>();
+ private final Set<Artifact> modularPrivateHeaders = new HashSet<>();
private final Set<Artifact> textualHeaders = new HashSet<>();
Builder setHeaderModule(Artifact.DerivedArtifact headerModule) {
@@ -1076,14 +1148,27 @@
return this;
}
- public Builder addHeaders(Collection<Artifact> headers) {
+ public Builder addPublicHeaders(Collection<Artifact> headers) {
// TODO(djasper): CPP_TEXTUAL_INCLUDEs are currently special cased here and in
// CppModuleMapAction. These should be moved to a place earlier in the Action construction.
for (Artifact header : headers) {
if (header.isFileType(CppFileTypes.CPP_TEXTUAL_INCLUDE)) {
this.textualHeaders.add(header);
} else {
- this.modularHeaders.add(header);
+ this.modularPublicHeaders.add(header);
+ }
+ }
+ return this;
+ }
+
+ public Builder addPrivateHeaders(Collection<Artifact> headers) {
+ // TODO(djasper): CPP_TEXTUAL_INCLUDEs are currently special cased here and in
+ // CppModuleMapAction. These should be moved to a place earlier in the Action construction.
+ for (Artifact header : headers) {
+ if (header.isFileType(CppFileTypes.CPP_TEXTUAL_INCLUDE)) {
+ this.textualHeaders.add(header);
+ } else {
+ this.modularPrivateHeaders.add(header);
}
}
return this;
@@ -1096,7 +1181,8 @@
/** Adds the headers of the given {@code HeaderInfo} into the one being built. */
public Builder mergeHeaderInfo(HeaderInfo otherHeaderInfo) {
- this.modularHeaders.addAll(otherHeaderInfo.modularHeaders);
+ this.modularPublicHeaders.addAll(otherHeaderInfo.modularPublicHeaders);
+ this.modularPrivateHeaders.addAll(otherHeaderInfo.modularPrivateHeaders);
this.textualHeaders.addAll(otherHeaderInfo.textualHeaders);
return this;
}
@@ -1105,7 +1191,8 @@
return new HeaderInfo(
headerModule,
picHeaderModule,
- ImmutableList.copyOf(modularHeaders),
+ ImmutableList.copyOf(modularPublicHeaders),
+ ImmutableList.copyOf(modularPrivateHeaders),
ImmutableList.copyOf(textualHeaders));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
index 6ab8714..c58cd96 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
@@ -986,8 +986,8 @@
ccCompilationContextBuilder.addDeclaredIncludeSrcs(privateHeaders);
ccCompilationContextBuilder.addDeclaredIncludeSrcs(additionalInputs);
ccCompilationContextBuilder.addNonCodeInputs(additionalInputs);
- ccCompilationContextBuilder.addModularHdrs(publicHeaders.getHeaders());
- ccCompilationContextBuilder.addModularHdrs(privateHeaders);
+ ccCompilationContextBuilder.addModularPublicHdrs(publicHeaders.getHeaders());
+ ccCompilationContextBuilder.addModularPrivateHdrs(privateHeaders);
ccCompilationContextBuilder.addTextualHdrs(publicTextualHeaders);
// Add this package's dir to declaredIncludeDirs, & this rule's headers to declaredIncludeSrcs
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
index 8dfda85..7322332 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -616,7 +616,7 @@
/* actionConstructionContext= */ null, /* configuration= */ null, /* label= */ null);
ImmutableList<Artifact> headerList = toNestedSetOfArtifacts(headers, "headers").toList();
ccCompilationContext.addDeclaredIncludeSrcs(headerList);
- ccCompilationContext.addModularHdrs(headerList);
+ ccCompilationContext.addModularPublicHdrs(headerList);
ccCompilationContext.addSystemIncludeDirs(
toNestedSetOfStrings(systemIncludes, "system_includes").toList().stream()
.map(x -> PathFragment.create(x))
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompilationContext.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompilationContext.java
index 3ed6e99..95420b3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompilationContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompilationContext.java
@@ -116,8 +116,8 @@
.addDeclaredIncludeSrcs(getPublicHeaders())
.addDeclaredIncludeSrcs(getPrivateHeaders())
.addDeclaredIncludeSrcs(getPublicTextualHeaders())
- .addModularHdrs(ImmutableList.copyOf(getPublicHeaders()))
- .addModularHdrs(ImmutableList.copyOf(getPrivateHeaders()))
+ .addModularPublicHdrs(ImmutableList.copyOf(getPublicHeaders()))
+ .addModularPrivateHdrs(ImmutableList.copyOf(getPrivateHeaders()))
.addTextualHdrs(ImmutableList.copyOf(getPublicTextualHeaders()))
.addIncludeDirs(getIncludes())
.addSystemIncludeDirs(getSystemIncludes())
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcCompilationContextApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcCompilationContextApi.java
index 9dbc65b..c329d9e 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcCompilationContextApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcCompilationContextApi.java
@@ -98,6 +98,22 @@
StarlarkList<FileT> getStarlarkDirectModularHeaders();
@StarlarkMethod(
+ name = "direct_public_headers",
+ doc =
+ "Returns the list of modular public headers (those listed in \"hdrs\") that are declared"
+ + " by this target.",
+ structField = true)
+ StarlarkList<FileT> getStarlarkDirectPublicHeaders();
+
+ @StarlarkMethod(
+ name = "direct_private_headers",
+ doc =
+ "Returns the list of modular private headers (those listed in \"srcs\") that are"
+ + " declared by this target.",
+ structField = true)
+ StarlarkList<FileT> getStarlarkDirectPrivateHeaders();
+
+ @StarlarkMethod(
name = "direct_textual_headers",
doc = "Returns the list of textual headers that are declared by this target.",
structField = true)
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
index e0f1463..2f67db7 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
@@ -6239,6 +6239,8 @@
" compilation_context = ctx.attr.dep[CcInfo].compilation_context",
" return [MyInfo(",
" direct_headers = compilation_context.direct_headers,",
+ " direct_public_headers = compilation_context.direct_public_headers,",
+ " direct_private_headers = compilation_context.direct_private_headers,",
" direct_textual_headers = compilation_context.direct_textual_headers,",
" )]",
"cc_info_extractor = rule(",
@@ -6286,9 +6288,25 @@
Iterable<Artifact> fooDirectHeaders = getArtifactsFromMyInfo(fooTarget, "direct_headers");
assertThat(baseArtifactNames(fooDirectHeaders)).containsExactly("foo.h", "foo_impl.h");
+ Iterable<Artifact> fooDirectPublicHeaders =
+ getArtifactsFromMyInfo(fooTarget, "direct_public_headers");
+ assertThat(baseArtifactNames(fooDirectPublicHeaders)).containsExactly("foo.h");
+
+ Iterable<Artifact> fooDirectPrivateHeaders =
+ getArtifactsFromMyInfo(fooTarget, "direct_private_headers");
+ assertThat(baseArtifactNames(fooDirectPrivateHeaders)).containsExactly("foo_impl.h");
+
ConfiguredTarget barTarget = getConfiguredTarget("//direct:bar");
Iterable<Artifact> barDirectHeaders = getArtifactsFromMyInfo(barTarget, "direct_headers");
assertThat(baseArtifactNames(barDirectHeaders)).containsExactly("bar.h");
+
+ Iterable<Artifact> barDirectPublicHeaders =
+ getArtifactsFromMyInfo(barTarget, "direct_public_headers");
+ assertThat(baseArtifactNames(barDirectPublicHeaders)).containsExactly("bar.h");
+
+ Iterable<Artifact> barDirectPrivateHeaders =
+ getArtifactsFromMyInfo(barTarget, "direct_private_headers");
+ assertThat(barDirectPrivateHeaders).isEmpty();
}
@Test
@@ -6395,8 +6413,10 @@
.map(CppModuleMap::getArtifact)
.collect(ImmutableList.toImmutableList())))
.containsExactly("public1.cppmap", "public2.cppmap");
- assertThat(baseArtifactNames(ccCompilationContext.getDirectHdrs()))
- .containsExactly("public1.h", "public2.h", "public1_impl.h", "public2_impl.h");
+ assertThat(baseArtifactNames(ccCompilationContext.getDirectPublicHdrs()))
+ .containsExactly("public1.h", "public2.h");
+ assertThat(baseArtifactNames(ccCompilationContext.getDirectPrivateHdrs()))
+ .containsExactly("public1_impl.h", "public2_impl.h");
assertThat(baseArtifactNames(ccCompilationContext.getTextualHdrs()))
.containsExactly("public1.inc", "public2.inc");
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
index b50b536..b2bce99 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
@@ -1931,8 +1931,10 @@
ConfiguredTarget target = getConfiguredTarget("//x:bar");
CcCompilationContext ccCompilationContext =
target.get(CcInfo.PROVIDER).getCcCompilationContext();
- assertThat(baseArtifactNames(ccCompilationContext.getDirectHdrs()))
- .containsExactly("bar.h", "bar_impl.h");
+ assertThat(baseArtifactNames(ccCompilationContext.getDirectPublicHdrs()))
+ .containsExactly("bar.h");
+ assertThat(baseArtifactNames(ccCompilationContext.getDirectPrivateHdrs()))
+ .containsExactly("bar_impl.h");
assertThat(baseArtifactNames(ccCompilationContext.getTextualHdrs())).containsExactly("bar.inc");
}
