From 23609fdc3b0e161835e59c761f68c06488f6cd4b Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Fri, 1 May 2020 14:35:57 -0700 Subject: [PATCH] Remove dead code for `mapAll` from `Args.add_all`. The implementation of `Args.add_all` has a flag for a `map_all` parameter with code supporting to evaluate and construct keys based on that, however it is always set to null, deeming all of the logic a no-op. Historically, it was used to implement `map_fn` for `Args.add`, but this has been dropped already. Delete the unused `map_all` parameter and code used to support it. PiperOrigin-RevId: 309475789 --- .../build/lib/analysis/skylark/Args.java | 4 -- .../skylark/StarlarkCustomCommandLine.java | 67 +------------------ 2 files changed, 3 insertions(+), 68 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java index 086b1667c4ee19..00e07fa02d9520 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java @@ -327,7 +327,6 @@ public CommandLineArgsApi addAll( addVectorArg( values, argName, - /* mapAll= */ null, mapEach != Starlark.NONE ? (StarlarkCallable) mapEach : null, formatEach != Starlark.NONE ? (String) formatEach : null, beforeEach != Starlark.NONE ? (String) beforeEach : null, @@ -367,7 +366,6 @@ public CommandLineArgsApi addJoined( addVectorArg( values, argName, - /* mapAll= */ null, mapEach != Starlark.NONE ? (StarlarkCallable) mapEach : null, formatEach != Starlark.NONE ? (String) formatEach : null, /* beforeEach= */ null, @@ -384,7 +382,6 @@ public CommandLineArgsApi addJoined( private void addVectorArg( Object value, String argName, - StarlarkCallable mapAll, StarlarkCallable mapEach, String formatEach, String beforeEach, @@ -417,7 +414,6 @@ private void addVectorArg( .setLocation(loc) .setArgName(argName) .setExpandDirectories(expandDirectories) - .setMapAll(mapAll) .setFormatEach(formatEach) .setBeforeEach(beforeEach) .setJoinWith(joinWith) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkCustomCommandLine.java index 96d8d75dfdf179..b54581a0fd6ea6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkCustomCommandLine.java @@ -15,7 +15,6 @@ import com.google.common.base.Joiner; import com.google.common.base.Objects; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; @@ -68,7 +67,7 @@ static final class VectorArg { private static final Interner interner = BlazeInterners.newStrongInterner(); private static final int HAS_LOCATION = 1; - private static final int HAS_MAP_ALL = 1 << 1; + // Deleted HAS_MAP_ALL = 1 << 1; private static final int HAS_MAP_EACH = 1 << 2; private static final int IS_NESTED_SET = 1 << 3; private static final int EXPAND_DIRECTORIES = 1 << 4; @@ -114,7 +113,6 @@ static VectorArg create(int features) { private static void push(ImmutableList.Builder arguments, Builder arg) { int features = 0; - features |= arg.mapAll != null ? HAS_MAP_ALL : 0; features |= arg.mapEach != null ? HAS_MAP_EACH : 0; features |= arg.nestedSet != null ? IS_NESTED_SET : 0; features |= arg.expandDirectories ? EXPAND_DIRECTORIES : 0; @@ -128,20 +126,13 @@ private static void push(ImmutableList.Builder arguments, Builder arg) { features |= arg.terminateWith != null ? HAS_TERMINATE_WITH : 0; boolean hasLocation = arg.location != null - && (features & (HAS_FORMAT_EACH | HAS_FORMAT_JOINED | HAS_MAP_ALL | HAS_MAP_EACH)) - != 0; + && (features & (HAS_FORMAT_EACH | HAS_FORMAT_JOINED | HAS_MAP_EACH)) != 0; features |= hasLocation ? HAS_LOCATION : 0; - Preconditions.checkState( - (features & (HAS_MAP_ALL | HAS_MAP_EACH)) != (HAS_MAP_ALL | HAS_MAP_EACH), - "Cannot use both map_all and map_each"); VectorArg vectorArg = VectorArg.create(features); arguments.add(vectorArg); if (hasLocation) { arguments.add(arg.location); } - if (arg.mapAll != null) { - arguments.add(arg.mapAll); - } if (arg.mapEach != null) { arguments.add(arg.mapEach); } @@ -185,8 +176,6 @@ private int eval( final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; final List originalValues; - StarlarkCallable mapAll = - ((features & HAS_MAP_ALL) != 0) ? (StarlarkCallable) arguments.get(argi++) : null; StarlarkCallable mapEach = ((features & HAS_MAP_EACH) != 0) ? (StarlarkCallable) arguments.get(argi++) : null; if ((features & IS_NESTED_SET) != 0) { @@ -208,33 +197,6 @@ private int eval( if (mapEach != null) { stringValues = new ArrayList<>(expandedValues.size()); applyMapEach(mapEach, expandedValues, stringValues::add, location, starlarkSemantics); - } else if (mapAll != null) { - Object result = applyMapFn(mapAll, expandedValues, location, starlarkSemantics); - if (!(result instanceof List)) { - throw new CommandLineExpansionException( - errorMessage( - "map_fn must return a list, got " + result.getClass().getSimpleName(), - location, - null)); - } - List resultAsList = (List) result; - if (resultAsList.size() != expandedValues.size()) { - throw new CommandLineExpansionException( - errorMessage( - String.format( - "map_fn must return a list of the same length as the input. " - + "Found list of length %d, expected %d.", - resultAsList.size(), expandedValues.size()), - location, - null)); - } - int count = resultAsList.size(); - stringValues = new ArrayList<>(count); - // map_fn contract doesn't guarantee that the values returned are strings, - // so convert here - for (int i = 0; i < count; ++i) { - stringValues.add(CommandLineItem.expandToCommandLine(resultAsList.get(i))); - } } else { int count = expandedValues.size(); stringValues = new ArrayList<>(expandedValues.size()); @@ -371,9 +333,6 @@ private int addToFingerprint( Fingerprint fingerprint, StarlarkSemantics starlarkSemantics) throws CommandLineExpansionException { - if ((features & HAS_MAP_ALL) != 0) { - return addToFingerprintLegacy(arguments, argi, fingerprint, starlarkSemantics); - } final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; StarlarkCallable mapEach = @@ -449,27 +408,12 @@ private int addToFingerprint( return argi; } - private int addToFingerprintLegacy( - List arguments, - int argi, - Fingerprint fingerprint, - StarlarkSemantics starlarkSemantics) - throws CommandLineExpansionException { - ImmutableList.Builder builder = ImmutableList.builder(); - argi = eval(arguments, argi, builder, null, starlarkSemantics); - for (String s : builder.build()) { - fingerprint.addString(s); - } - return argi; - } - static class Builder { @Nullable private final Sequence list; @Nullable private final NestedSet nestedSet; private Location location; public String argName; private boolean expandDirectories; - private StarlarkCallable mapAll; private StarlarkCallable mapEach; private String formatEach; private String beforeEach; @@ -504,11 +448,6 @@ Builder setExpandDirectories(boolean expandDirectories) { return this; } - Builder setMapAll(StarlarkCallable mapAll) { - this.mapAll = mapAll; - return this; - } - Builder setMapEach(StarlarkCallable mapEach) { this.mapEach = mapEach; return this; @@ -943,7 +882,7 @@ static class FilesetSymlinkFile implements FileApi, CommandLineItem { private final Artifact fileset; private final PathFragment execPath; - public FilesetSymlinkFile(Artifact fileset, PathFragment execPath) { + FilesetSymlinkFile(Artifact fileset, PathFragment execPath) { this.fileset = fileset; this.execPath = execPath; }