diff options
8 files changed, 53 insertions, 51 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java index 5140f572b1..9e26da0633 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansion.java @@ -14,7 +14,7 @@ package com.google.devtools.build.lib.runtime; -import com.google.common.collect.Iterables; +import com.google.common.collect.ImmutableList; import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.ExpansionFunction; import com.google.devtools.common.options.IsolatedOptionsData; @@ -147,7 +147,7 @@ public class AllIncompatibleChangesExpansion implements ExpansionFunction { } @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { // Grab all registered options that are identified as incompatible changes by either name or // by category. Ensure they satisfy our requirements. ArrayList<String> incompatibleChanges = new ArrayList<>(); @@ -163,6 +163,6 @@ public class AllIncompatibleChangesExpansion implements ExpansionFunction { // Sort to get a deterministic canonical order. This probably isn't necessary because the // options parser will do its own sorting when canonicalizing, but it seems like it can't hurt. incompatibleChanges.sort(null); - return Iterables.toArray(incompatibleChanges, String.class); + return ImmutableList.copyOf(incompatibleChanges); } } diff --git a/src/main/java/com/google/devtools/common/options/ExpansionFunction.java b/src/main/java/com/google/devtools/common/options/ExpansionFunction.java index ffab6e70fc..be4773ec39 100644 --- a/src/main/java/com/google/devtools/common/options/ExpansionFunction.java +++ b/src/main/java/com/google/devtools/common/options/ExpansionFunction.java @@ -13,9 +13,11 @@ // limitations under the License. package com.google.devtools.common.options; +import com.google.common.collect.ImmutableList; + /** - * A function from an option parser's static setup (what flags it knows about) to an expansion - * String[] to use for one of its options. + * A function from an option parser's static setup (what flags it knows about) to a list of + * expansion Strings to use for one of its options. */ public interface ExpansionFunction { @@ -27,5 +29,5 @@ public interface ExpansionFunction { * information is computed * @return An expansion to use for all occurrences of this option in this parser */ - public String[] getExpansion(IsolatedOptionsData optionsData); + ImmutableList<String> getExpansion(IsolatedOptionsData optionsData); } diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java index e71321c23f..ba53172211 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -14,6 +14,7 @@ package com.google.devtools.common.options; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Maps; import java.lang.reflect.Constructor; @@ -31,28 +32,25 @@ import javax.annotation.concurrent.Immutable; @Immutable final class OptionsData extends IsolatedOptionsData { - /** - * Mapping from each Option-annotated field with a {@code String[]} expansion to that expansion. - */ - // TODO(brandjon): This is technically not necessarily immutable due to String[], and should use - // ImmutableList. Either fix this or remove @Immutable. - private final ImmutableMap<Field, String[]> evaluatedExpansions; + /** Mapping from each Option-annotated field to expansion strings, if it has any. */ + private final ImmutableMap<Field, ImmutableList<String>> evaluatedExpansions; /** Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info. */ - private OptionsData(IsolatedOptionsData base, Map<Field, String[]> evaluatedExpansions) { + private OptionsData( + IsolatedOptionsData base, Map<Field, ImmutableList<String>> evaluatedExpansions) { super(base); this.evaluatedExpansions = ImmutableMap.copyOf(evaluatedExpansions); } - private static final String[] EMPTY_EXPANSION = new String[] {}; + private static final ImmutableList<String> EMPTY_EXPANSION = ImmutableList.<String>of(); /** * Returns the expansion of an options field, regardless of whether it was defined using {@link * Option#expansion} or {@link Option#expansionFunction}. If the field is not an expansion option, * returns an empty array. */ - public String[] getEvaluatedExpansion(Field field) { - String[] result = evaluatedExpansions.get(field); + public ImmutableList<String> getEvaluatedExpansion(Field field) { + ImmutableList<String> result = evaluatedExpansions.get(field); return result != null ? result : EMPTY_EXPANSION; } @@ -65,7 +63,7 @@ final class OptionsData extends IsolatedOptionsData { IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes); // All that's left is to compute expansions. - Map<Field, String[]> evaluatedExpansionsBuilder = Maps.newHashMap(); + Map<Field, ImmutableList<String>> evaluatedExpansionsBuilder = Maps.newHashMap(); for (Map.Entry<String, Field> entry : isolatedData.getAllNamedFields()) { Field field = entry.getValue(); Option annotation = field.getAnnotation(Option.class); @@ -76,7 +74,7 @@ final class OptionsData extends IsolatedOptionsData { throw new AssertionError( "Cannot set both expansion and expansionFunction for option --" + annotation.name()); } else if (constExpansion.length > 0) { - evaluatedExpansionsBuilder.put(field, constExpansion); + evaluatedExpansionsBuilder.put(field, ImmutableList.copyOf(constExpansion)); } else if (usesExpansionFunction(annotation)) { if (Modifier.isAbstract(expansionFunctionClass.getModifiers())) { throw new AssertionError( @@ -92,7 +90,7 @@ final class OptionsData extends IsolatedOptionsData { // time it is used. throw new AssertionError(e); } - String[] expansion = instance.getExpansion(isolatedData); + ImmutableList<String> expansion = instance.getExpansion(isolatedData); evaluatedExpansionsBuilder.put(field, expansion); } } diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java index f80f1baf11..e0fc06249e 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -206,12 +206,12 @@ class OptionsParserImpl { result.add( new OptionValueDescription( fieldName, - /* originalValueString */null, + /*originalValueString=*/ null, value, OptionPriority.DEFAULT, - /* source */ null, - /* implicitDependant */ null, - /* expandedFrom */ null, + /*source=*/ null, + /*implicitDependant=*/ null, + /*expandedFrom=*/ null, false)); } else { result.add(entry); @@ -355,19 +355,19 @@ class OptionsParserImpl { /* expandedFrom */ name, /* implicitDependant */ null), getExpansionDescriptions( - optionAnnotation.implicitRequirements(), + ImmutableList.copyOf(optionAnnotation.implicitRequirements()), /* expandedFrom */ null, /* implicitDependant */ name)); } /** - * @return A list of the descriptions corresponding to the list of unparsed flags passed in. - * These descriptions are are divorced from the command line - there is no correct priority or - * source for these, as they are not actually set values. The value itself is also a string, no - * conversion has taken place. + * @return A list of the descriptions corresponding to the list of unparsed flags passed in. These + * descriptions are are divorced from the command line - there is no correct priority or + * source for these, as they are not actually set values. The value itself is also a string, + * no conversion has taken place. */ private ImmutableList<OptionValueDescription> getExpansionDescriptions( - String[] optionStrings, String expandedFrom, String implicitDependant) + ImmutableList<String> optionStrings, String expandedFrom, String implicitDependant) throws OptionsParsingException { ImmutableList.Builder<OptionValueDescription> builder = ImmutableList.builder(); ImmutableList<String> options = ImmutableList.copyOf(optionStrings); @@ -498,8 +498,8 @@ class OptionsParserImpl { } // Handle expansion options. - String[] expansion = optionsData.getEvaluatedExpansion(field); - if (expansion.length > 0) { + ImmutableList<String> expansion = optionsData.getEvaluatedExpansion(field); + if (!expansion.isEmpty()) { Function<Object, String> expansionSourceFunction = Functions.constant( "expanded from option --" @@ -507,8 +507,8 @@ class OptionsParserImpl { + " from " + sourceFunction.apply(originalName)); maybeAddDeprecationWarning(field); - List<String> unparsed = parse(priority, expansionSourceFunction, null, originalName, - ImmutableList.copyOf(expansion)); + List<String> unparsed = + parse(priority, expansionSourceFunction, null, originalName, expansion); if (!unparsed.isEmpty()) { // Throw an assertion, because this indicates an error in the code that specified the // expansion for the current option. diff --git a/src/main/java/com/google/devtools/common/options/OptionsUsage.java b/src/main/java/com/google/devtools/common/options/OptionsUsage.java index 1c9f5193f2..742e9f98f8 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java +++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java @@ -16,6 +16,7 @@ package com.google.devtools.common.options; import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; import com.google.common.escape.Escaper; import java.lang.reflect.Field; import java.text.BreakIterator; @@ -83,9 +84,9 @@ class OptionsUsage { * object is supplied, the expansion is read from that. Otherwise, the annotation is inspected: If * the annotation uses {@link Option#expansion} it is returned, and if it uses {@link * Option#expansionFunction} null is returned, indicating a lack of definite information. In all - * cases, when the option is not an expansion option, an empty array is returned. + * cases, when the option is not an expansion option, an empty list is returned. */ - private static @Nullable String[] getExpansionIfKnown( + private static @Nullable ImmutableList<String> getExpansionIfKnown( Field optionField, Option annotation, @Nullable OptionsData optionsData) { if (optionsData != null) { return optionsData.getEvaluatedExpansion(optionField); @@ -93,8 +94,8 @@ class OptionsUsage { if (OptionsData.usesExpansionFunction(annotation)) { return null; } else { - // Empty array if it's not an expansion option. - return annotation.expansion(); + // Empty list if it's not an expansion option. + return ImmutableList.copyOf(annotation.expansion()); } } } @@ -142,10 +143,10 @@ class OptionsUsage { usage.append(paragraphFill(annotation.help(), 4, 80)); // (indent, width) usage.append('\n'); } - String[] expansion = getExpansionIfKnown(optionField, annotation, optionsData); + ImmutableList<String> expansion = getExpansionIfKnown(optionField, annotation, optionsData); if (expansion == null) { usage.append(" Expands to unknown options.\n"); - } else if (expansion.length > 0) { + } else if (!expansion.isEmpty()) { StringBuilder expandsMsg = new StringBuilder("Expands to: "); for (String exp : expansion) { expandsMsg.append(exp).append(" "); @@ -200,10 +201,10 @@ class OptionsUsage { usage.append(paragraphFill(escaper.escape(annotation.help()), 0, 80)); // (indent, width) usage.append('\n'); } - String[] expansion = getExpansionIfKnown(optionField, annotation, optionsData); + ImmutableList<String> expansion = getExpansionIfKnown(optionField, annotation, optionsData); if (expansion == null) { usage.append(" Expands to unknown options.<br>\n"); - } else if (expansion.length > 0) { + } else if (!expansion.isEmpty()) { usage.append("<br/>\n"); StringBuilder expandsMsg = new StringBuilder("Expands to:<br/>\n"); for (String exp : expansion) { diff --git a/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java index 0048e0905b..8d6788e063 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/AllIncompatibleChangesExpansionTest.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.runtime; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.UseDefault; @@ -85,8 +86,8 @@ public class AllIncompatibleChangesExpansionTest { /** Dummy comment (linter suppression) */ public static class YExpansion implements ExpansionFunction { @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { - return new String[] {"--noY"}; + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { + return ImmutableList.of("--noY"); } } diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index 91de13774f..d2e2faec1e 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -868,8 +868,8 @@ public class OptionsParserTest { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { - return new String[] {"--yyy"}; + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { + return ImmutableList.of("--yyy"); } } @@ -900,7 +900,7 @@ public class OptionsParserTest { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { return null; } } @@ -937,8 +937,8 @@ public class OptionsParserTest { /** ExpFunc */ public static class ExpFunc implements ExpansionFunction { @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { - return new String[] {"--expands"}; + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { + return ImmutableList.of("--expands"); } } diff --git a/src/test/java/com/google/devtools/common/options/OptionsTest.java b/src/test/java/com/google/devtools/common/options/OptionsTest.java index 24b435f614..dbe680da5a 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java @@ -18,7 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static java.util.Arrays.asList; import static org.junit.Assert.fail; -import com.google.common.collect.Iterables; +import com.google.common.collect.ImmutableList; import com.google.common.testing.EqualsTester; import java.net.MalformedURLException; import java.net.URL; @@ -71,14 +71,14 @@ public class OptionsTest { /** SpecialExpansion */ public static class SpecialExpansion implements ExpansionFunction { @Override - public String[] getExpansion(IsolatedOptionsData optionsData) { + public ImmutableList<String> getExpansion(IsolatedOptionsData optionsData) { TreeSet<String> flags = new TreeSet<>(); for (Map.Entry<String, ?> entry : optionsData.getAllNamedFields()) { if (entry.getKey().startsWith("specialexp_")) { flags.add("--" + entry.getKey()); } } - return Iterables.toArray(flags, String.class); + return ImmutableList.copyOf(flags); } } |