Get color diagnostic option directly from passed in tool

Summary: Instead of passing it down via preprocessor/compiler delegate, into ToolCommand, into Step, just get the args from the tool, which is already passed into the step. Test Plan: CI Reviewed By: beefon fbshipit-source-id: 18d6399
classicvalues · May 15, 2017 · 3286637 · 3286637
1 parent 6a85182
commit 3286637
Show file tree

Hide file tree

Showing 6 changed files with 14 additions and 42 deletions.
diff --git a/src/com/facebook/buck/cxx/CompilerDelegate.java b/src/com/facebook/buck/cxx/CompilerDelegate.java
@@ -24,7 +24,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Ordering;
 import java.nio.file.Path;
-import java.util.Optional;
 
 /** Helper class for generating compiler invocations for a cxx compilation rule. */
 class CompilerDelegate implements RuleKeyAppendable {
@@ -90,10 +89,6 @@ public ImmutableList<SourcePath> getInputsAfterBuildingLocally() {
     return Ordering.natural().immutableSortedCopy(compiler.getInputs());
   }
 
-  public Optional<ImmutableList<String>> getFlagsForColorDiagnostics() {
-    return compiler.getFlagsForColorDiagnostics();
-  }
-
   public boolean isArgFileSupported() {
     return compiler.isArgFileSupported();
   }

diff --git a/src/com/facebook/buck/cxx/CxxPrecompiledHeader.java b/src/com/facebook/buck/cxx/CxxPrecompiledHeader.java
@@ -246,8 +246,7 @@ CxxPreprocessAndCompileStep makeMainStep(SourcePathResolver resolver, Path scrat
                             compilerFlags, /* no pch */ Optional.empty()))
                     .build()
                     .getAllFlags()),
-            preprocessorDelegate.getEnvironment(),
-            preprocessorDelegate.getFlagsForColorDiagnostics()),
+            preprocessorDelegate.getEnvironment()),
         preprocessorDelegate.getHeaderPathNormalizer(),
         compilerSanitizer,
         scratchDir,

diff --git a/src/com/facebook/buck/cxx/CxxPreprocessAndCompile.java b/src/com/facebook/buck/cxx/CxxPreprocessAndCompile.java
@@ -200,10 +200,7 @@ CxxPreprocessAndCompileStep makeMainStep(
         getRelativeInputPath(resolver),
         inputType,
         new CxxPreprocessAndCompileStep.ToolCommand(
-            compilerDelegate.getCommandPrefix(),
-            arguments,
-            compilerDelegate.getEnvironment(),
-            compilerDelegate.getFlagsForColorDiagnostics()),
+            compilerDelegate.getCommandPrefix(), arguments, compilerDelegate.getEnvironment()),
         headerPathNormalizer,
         sanitizer,
         scratchDir,

diff --git a/src/com/facebook/buck/cxx/CxxPreprocessAndCompileStep.java b/src/com/facebook/buck/cxx/CxxPreprocessAndCompileStep.java
@@ -140,7 +140,12 @@ ImmutableList<String> getArguments(boolean allowColorsInDiagnostics) {
             ? inputType.getPrecompiledHeaderLanguage().get()
             : inputType.getLanguage();
     return ImmutableList.<String>builder()
-        .addAll(command.getArguments(allowColorsInDiagnostics))
+        .addAll(command.getArguments())
+        .addAll(
+            (allowColorsInDiagnostics
+                    ? compiler.getFlagsForColorDiagnostics()
+                    : Optional.<ImmutableList<String>>empty())
+                .orElseGet(ImmutableList::of))
         .addAll(compiler.languageArgs(inputLanguage))
         .addAll(sanitizer.getCompilationFlags())
         .add("-c")
@@ -211,7 +216,7 @@ private int executeCompilation(ExecutionContext context) throws Exception {
           .post(
               createConsoleEvent(
                   context,
-                  command.supportsColorsInDiagnostics(),
+                  compiler.getFlagsForColorDiagnostics().isPresent(),
                   exitCode == 0 ? Level.WARNING : Level.SEVERE,
                   err));
     }
@@ -307,40 +312,26 @@ public static class ToolCommand {
     private final ImmutableList<String> commandPrefix;
     private final ImmutableList<String> arguments;
     private final ImmutableMap<String, String> environment;
-    private final Optional<ImmutableList<String>> flagsForColorDiagnostics;
 
     public ToolCommand(
         ImmutableList<String> commandPrefix,
         ImmutableList<String> arguments,
-        ImmutableMap<String, String> environment,
-        Optional<ImmutableList<String>> flagsForColorDiagnostics) {
+        ImmutableMap<String, String> environment) {
       this.commandPrefix = commandPrefix;
       this.arguments = arguments;
       this.environment = environment;
-      this.flagsForColorDiagnostics = flagsForColorDiagnostics;
     }
 
     public ImmutableList<String> getCommandPrefix() {
       return commandPrefix;
     }
 
-    public ImmutableList<String> getArguments(boolean allowColorsInDiagnostics) {
-      if (allowColorsInDiagnostics && flagsForColorDiagnostics.isPresent()) {
-        return ImmutableList.<String>builder()
-            .addAll(arguments)
-            .addAll(flagsForColorDiagnostics.get())
-            .build();
-      } else {
-        return arguments;
-      }
+    public ImmutableList<String> getArguments() {
+      return arguments;
     }
 
     public ImmutableMap<String, String> getEnvironment() {
       return environment;
     }
-
-    public boolean supportsColorsInDiagnostics() {
-      return flagsForColorDiagnostics.isPresent();
-    }
   }
 }
diff --git a/src/com/facebook/buck/cxx/PreprocessorDelegate.java b/src/com/facebook/buck/cxx/PreprocessorDelegate.java
@@ -271,10 +271,6 @@ public Predicate<SourcePath> getCoveredByDepFilePredicate() {
     return (SourcePath path) -> true;
   }
 
-  public Optional<ImmutableList<String>> getFlagsForColorDiagnostics() {
-    return preprocessor.getFlagsForColorDiagnostics();
-  }
-
   public HeaderVerification getHeaderVerification() {
     return headerVerification;
   }

diff --git a/test/com/facebook/buck/cxx/CxxCompileStepIntegrationTest.java b/test/com/facebook/buck/cxx/CxxCompileStepIntegrationTest.java
@@ -88,10 +88,7 @@ private void assertCompDir(Path compDir, Optional<String> failure) throws Except
             relativeInput,
             CxxSource.Type.C,
             new CxxPreprocessAndCompileStep.ToolCommand(
-                compilerCommandPrefix,
-                compilerArguments.build(),
-                ImmutableMap.of(),
-                Optional.empty()),
+                compilerCommandPrefix, compilerArguments.build(), ImmutableMap.of()),
             HeaderPathNormalizer.empty(pathResolver),
             sanitizer,
             scratchDir,
@@ -160,10 +157,7 @@ public void createsAnArgfile() throws Exception {
             relativeInput,
             CxxSource.Type.C,
             new CxxPreprocessAndCompileStep.ToolCommand(
-                compilerCommandPrefix,
-                compilerArguments.build(),
-                ImmutableMap.of(),
-                Optional.empty()),
+                compilerCommandPrefix, compilerArguments.build(), ImmutableMap.of()),
             HeaderPathNormalizer.empty(pathResolver),
             CxxPlatformUtils.DEFAULT_COMPILER_DEBUG_PATH_SANITIZER,
             scratchDir,