1 files changed, 43 insertions, 45 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 1825742e82..2ede292346 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -41,7 +41,6 @@ import com.google.devtools.build.lib.rules.test.InstrumentedFilesProviderImpl;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
-import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -60,13 +59,6 @@ public final class CcCommon {
 
   private static final String NO_COPTS_ATTRIBUTE = "nocopts";
 
-  private static final FileTypeSet SOURCE_TYPES = FileTypeSet.of(
-      CppFileTypes.CPP_SOURCE,
-      CppFileTypes.CPP_HEADER,
-      CppFileTypes.C_SOURCE,
-      CppFileTypes.ASSEMBLER,
-      CppFileTypes.ASSEMBLER_WITH_C_PREPROCESSOR);
-
   /**
    * Collects all metadata files generated by C++ compilation actions that output the .o files
    * on the input.
@@ -97,15 +89,11 @@ public final class CcCommon {
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
   
-  /** Active toolchain features. */
-  private final FeatureConfiguration featureConfiguration;
-
   private final RuleContext ruleContext;
 
-  public CcCommon(RuleContext ruleContext, FeatureConfiguration featureConfiguration) {
+  public CcCommon(RuleContext ruleContext) {
     this.ruleContext = ruleContext;
     this.cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
-    this.featureConfiguration = featureConfiguration;
   }
 
   /**
@@ -201,39 +189,23 @@ public final class CcCommon {
     return new TransitiveLipoInfoProvider(scannableBuilder.build());
   }
 
-  private boolean shouldProcessHeaders() {
-    return featureConfiguration.isEnabled(CppRuleClasses.PREPROCESS_HEADERS)
-        || featureConfiguration.isEnabled(CppRuleClasses.PARSE_HEADERS);
-  }
-
   /**
    * Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input
    * source file and the label of the rule that generates it (or the label of the source file
-   * itself if it is an input file)
+   * itself if it is an input file).
    */
-  ImmutableList<Pair<Artifact, Label>> getCAndCppSources() {
+  List<Pair<Artifact, Label>> getSources() {
     Map<Artifact, Label> map = Maps.newLinkedHashMap();
     Iterable<FileProvider> providers =
         ruleContext.getPrerequisites("srcs", Mode.TARGET, FileProvider.class);
-    // TODO(bazel-team): Move header processing logic down in the stack (to CcLibraryHelper or
-    // such).
-    boolean processHeaders = shouldProcessHeaders();
-    if (processHeaders && hasAttribute("hdrs", BuildType.LABEL_LIST)) {
-      providers = Iterables.concat(providers,
-          ruleContext.getPrerequisites("hdrs", Mode.TARGET, FileProvider.class));
-    }
     for (FileProvider provider : providers) {
-      for (Artifact artifact : FileType.filter(provider.getFilesToBuild(), SOURCE_TYPES)) {
-        boolean isHeader = CppFileTypes.CPP_HEADER.matches(artifact.getExecPath());
-        if ((isHeader && !processHeaders)
-            || CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(artifact.getExecPath())) {
-          continue;
-        }
-        Label oldLabel = map.put(artifact, provider.getLabel());
+      for (Artifact artifact : provider.getFilesToBuild()) {
         // TODO(bazel-team): We currently do not warn for duplicate headers with
         // different labels, as that would require cleaning up the code base
         // without significant benefit; we should eventually make this
         // consistent one way or the other.
+        Label oldLabel = map.put(artifact, provider.getLabel());
+        boolean isHeader = CppFileTypes.CPP_HEADER.matches(artifact.getExecPath());
         if (!isHeader && oldLabel != null && !oldLabel.equals(provider.getLabel())) {
           ruleContext.attributeError("srcs", String.format(
               "Artifact '%s' is duplicated (through '%s' and '%s')",
@@ -246,7 +218,6 @@ public final class CcCommon {
     for (Map.Entry<Artifact, Label> entry : map.entrySet()) {
       result.add(Pair.of(entry.getKey(), entry.getValue()));
     }
-
     return result.build();
   }
 
@@ -255,21 +226,44 @@ public final class CcCommon {
    * warnings to the {@link RuleContext} as a side effect, and so should only be called once for any
    * given rule.
    */
-  public static List<Artifact> getHeaders(RuleContext ruleContext) {
-    List<Artifact> hdrs = new ArrayList<>();
+  public static List<Pair<Artifact, Label>> getHeaders(RuleContext ruleContext) {
+    Map<Artifact, Label> map = Maps.newLinkedHashMap();
     for (TransitiveInfoCollection target :
         ruleContext.getPrerequisitesIf("hdrs", Mode.TARGET, FileProvider.class)) {
       FileProvider provider = target.getProvider(FileProvider.class);
       for (Artifact artifact : provider.getFilesToBuild()) {
-        if (!CppRuleClasses.DISALLOWED_HDRS_FILES.matches(artifact.getFilename())) {
-          hdrs.add(artifact);
-        } else {
+        if (CppRuleClasses.DISALLOWED_HDRS_FILES.matches(artifact.getFilename())) {
           ruleContext.attributeWarning("hdrs", "file '" + artifact.getFilename()
               + "' from target '" + target.getLabel() + "' is not allowed in hdrs");
+          continue;
+        }
+        Label oldLabel = map.put(artifact, provider.getLabel());
+        if (oldLabel != null && !oldLabel.equals(provider.getLabel())) {
+          ruleContext.attributeWarning(
+              "hdrs",
+              String.format(
+                  "Artifact '%s' is duplicated (through '%s' and '%s')",
+                  artifact.getExecPathString(),
+                  oldLabel,
+                  provider.getLabel()));
         }
       }
     }
-    return hdrs;
+    
+    ImmutableList.Builder<Pair<Artifact, Label>> result = ImmutableList.builder();
+    for (Map.Entry<Artifact, Label> entry : map.entrySet()) {
+      result.add(Pair.of(entry.getKey(), entry.getValue()));
+    }
+    return result.build();
+  }
+
+  /**
+   * Returns the files from headers and does some sanity checks. Note that this method reports
+   * warnings to the {@link RuleContext} as a side effect, and so should only be called once for any
+   * given rule.
+   */
+  public List<Pair<Artifact, Label>> getHeaders() {
+    return getHeaders(ruleContext);
   }
 
   private static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
@@ -423,12 +417,16 @@ public final class CcCommon {
    */
   static NestedSet<Artifact> collectCompilationPrerequisites(
       RuleContext ruleContext, CppCompilationContext context) {
-    // TODO(bazel-team): Use context.getCompilationPrerequisites() instead.
+    // TODO(bazel-team): Use context.getCompilationPrerequisites() instead; note that this will
+    // need cleaning up the prerequisites, as the compilation context currently collects them
+    // transitively (to get transitive headers), but source files are not transitive compilation
+    // prerequisites.
     NestedSetBuilder<Artifact> prerequisites = NestedSetBuilder.stableOrder();
     if (ruleContext.attributes().has("srcs", BuildType.LABEL_LIST)) {
-      for (FileProvider provider : ruleContext
-          .getPrerequisites("srcs", Mode.TARGET, FileProvider.class)) {
-        prerequisites.addAll(FileType.filter(provider.getFilesToBuild(), SOURCE_TYPES));
+      for (FileProvider provider :
+          ruleContext.getPrerequisites("srcs", Mode.TARGET, FileProvider.class)) {
+        prerequisites.addAll(
+            FileType.filter(provider.getFilesToBuild(), CcLibraryHelper.SOURCE_TYPES));
       }
     }
     prerequisites.addTransitive(context.getDeclaredIncludeSrcs());