From 938e2948324b44ff66b4ef1dc8d949ecf15e3d2d Mon Sep 17 00:00:00 2001 From: L?szl? Csomor Date: Tue, 28 Nov 2017 06:59:09 -0800 Subject: refactor: remove unnecessary Enums from Expander These enums were only used inside the class and only to slighlty improve readability. The Tokenize enum was a simple boolean. The Options enum was implementing two boolean flags, and it improved call-site readability at the cost of implementation-site readability and checking if the value is set via set-containment. Change-Id: I3858ff0c67f89c8b2c5631e260ce79cd939c6eb1 PiperOrigin-RevId: 177155294 --- .../devtools/build/lib/analysis/Expander.java | 58 ++++++++-------------- .../lib/analysis/LocationTemplateContext.java | 25 ++-------- 2 files changed, 25 insertions(+), 58 deletions(-) (limited to 'src/main/java') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Expander.java b/src/main/java/com/google/devtools/build/lib/analysis/Expander.java index 60aa6935b5..0e40e1e54f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Expander.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Expander.java @@ -31,19 +31,6 @@ import javax.annotation.Nullable; * Expansion of strings and string lists by replacing make variables and $(location) functions. */ public final class Expander { - /** Indicates whether a string list attribute should be tokenized. */ - private enum Tokenize { - YES, - NO - } - - /** List of options to tweak the LocationExpander. */ - public static enum Options { - /** output the execPath instead of the relative path */ - EXEC_PATHS, - /** Allow to take label from the data attribute */ - ALLOW_DATA, - } private final RuleContext ruleContext; private final TemplateContext templateContext; @@ -54,38 +41,38 @@ public final class Expander { } /** - * Returns a new instance that also expands locations using the default configuration of - * {@link LocationTemplateContext}. + * Returns a new instance that also expands locations using the default configuration of {@link + * LocationTemplateContext}. */ - public Expander withLocations(Options... options) { + private Expander withLocations(boolean execPaths, boolean allowData) { TemplateContext newTemplateContext = - new LocationTemplateContext(templateContext, ruleContext, options); + new LocationTemplateContext(templateContext, ruleContext, null, execPaths, allowData); return new Expander(ruleContext, newTemplateContext); } /** - * Returns a new instance that also expands locations, passing {@link Options#ALLOW_DATA} to the - * underlying {@link LocationTemplateContext}. + * Returns a new instance that also expands locations, passing {@code allowData} to the underlying + * {@link LocationTemplateContext}. */ public Expander withDataLocations() { - return withLocations(Options.ALLOW_DATA); + return withLocations(false, true); } /** - * Returns a new instance that also expands locations, passing {@link Options#ALLOW_DATA} and - * {@link Options#EXEC_PATHS} to the underlying {@link LocationTemplateContext}. + * Returns a new instance that also expands locations, passing {@code allowData} and {@code + * execPaths} to the underlying {@link LocationTemplateContext}. */ public Expander withDataExecLocations() { - return withLocations(Options.ALLOW_DATA, Options.EXEC_PATHS); + return withLocations(true, true); } /** * Returns a new instance that also expands locations, passing the given location map, as well as - * {@link Options#EXEC_PATHS} to the underlying {@link LocationTemplateContext}. + * {@code execPaths} to the underlying {@link LocationTemplateContext}. */ public Expander withExecLocations(ImmutableMap> locations) { TemplateContext newTemplateContext = - new LocationTemplateContext(templateContext, ruleContext, locations, Options.EXEC_PATHS); + new LocationTemplateContext(templateContext, ruleContext, locations, true, false); return new Expander(ruleContext, newTemplateContext); } @@ -97,19 +84,14 @@ public final class Expander { List result, String attributeName, String value) { - expandValue(result, attributeName, value, Tokenize.YES); + expandValue(result, attributeName, value, /* shouldTokenize */ true); } - /** - * Expands make variables and $(location) tags in value, and optionally tokenizes the result. - */ + /** Expands make variables and $(location) tags in value, and optionally tokenizes the result. */ private void expandValue( - List tokens, - String attributeName, - String value, - Tokenize tokenize) { + List tokens, String attributeName, String value, boolean shouldTokenize) { value = expand(attributeName, value); - if (tokenize == Tokenize.YES) { + if (shouldTokenize) { try { ShellUtils.tokenize(tokens, value); } catch (ShellUtils.TokenizationException e) { @@ -160,10 +142,10 @@ public final class Expander { * attribute name is only used for error reporting. */ private ImmutableList expandAndTokenizeList( - String attrName, List values, Tokenize tokenize) { + String attrName, List values, boolean shouldTokenize) { List variables = new ArrayList<>(); for (String variable : values) { - expandValue(variables, attrName, variable, tokenize); + expandValue(variables, attrName, variable, shouldTokenize); } return ImmutableList.copyOf(variables); } @@ -181,7 +163,7 @@ public final class Expander { * Expands all the strings in the given list. The attribute name is only used for error reporting. */ public ImmutableList list(String attrName, List values) { - return expandAndTokenizeList(attrName, values, Tokenize.NO); + return expandAndTokenizeList(attrName, values, /* shouldTokenize */ false); } /** @@ -197,7 +179,7 @@ public final class Expander { * name is only used for error reporting. */ public ImmutableList tokenized(String attrName, List values) { - return expandAndTokenizeList(attrName, values, Tokenize.YES); + return expandAndTokenizeList(attrName, values, /* shouldTokenize */ true); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java b/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java index 8817f42f72..3deee31c50 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java @@ -18,9 +18,7 @@ import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.analysis.Expander.Options; import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException; import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; import com.google.devtools.build.lib.cmdline.Label; @@ -59,32 +57,19 @@ final class LocationTemplateContext implements TemplateContext { this.functions = LocationExpander.allLocationFunctions(root, locationMap, execPaths); } - private LocationTemplateContext( + public LocationTemplateContext( TemplateContext delegate, RuleContext ruleContext, @Nullable ImmutableMap> labelMap, - ImmutableSet options) { + boolean execPaths, + boolean allowData) { this( delegate, ruleContext.getLabel(), // Use a memoizing supplier to avoid eagerly building the location map. Suppliers.memoize( - () -> LocationExpander.buildLocationMap( - ruleContext, labelMap, options.contains(Options.ALLOW_DATA))), - options.contains(Options.EXEC_PATHS)); - } - - public LocationTemplateContext( - TemplateContext delegate, - RuleContext ruleContext, - @Nullable ImmutableMap> labelMap, - Options... options) { - this(delegate, ruleContext, labelMap, ImmutableSet.copyOf(options)); - } - - public LocationTemplateContext( - TemplateContext delegate, RuleContext ruleContext, Options... options) { - this(delegate, ruleContext, null, ImmutableSet.copyOf(options)); + () -> LocationExpander.buildLocationMap(ruleContext, labelMap, allowData)), + execPaths); } @Override -- cgit v1.2.3