From 9b48c841eee8be5a30114c6d8d33f10d8bc8b50b Mon Sep 17 00:00:00 2001 From: Roberto Lublinerman Date: Mon, 12 Jan 2015 14:26:28 -0800 Subject: [PATCH] Small cleanups. Change-Id: I94f54b225bc1dca31493c86eaba91830f44c4378 --- .../impl/CompileTimeConstantsReplacer.java | 25 ++--- .../google/gwt/util/tools/ArgHandlerEnum.java | 106 ++++++++++-------- .../gwt/util/tools/ArgHandlerEnumTest.java | 50 ++++++--- 3 files changed, 106 insertions(+), 75 deletions(-) diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/CompileTimeConstantsReplacer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/CompileTimeConstantsReplacer.java index 4a394aceaa3..0491e08a7b6 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/CompileTimeConstantsReplacer.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/CompileTimeConstantsReplacer.java @@ -52,23 +52,22 @@ private JExpression resolveFieldValue(JField field) { JExpression value = resolveValuesByField.get(field); if (value != null) { return new CloneExpressionVisitor().cloneExpression(value); - } else { - // TODO(rluble): Simplify the expression to a literal here after refactoring Simplifier. - // The initializer is guaranteed to be constant but it may be a non literal expression, so - // clone it and recursively remove field references, and finally cache the results. - value = accept(new CloneExpressionVisitor().cloneExpression(field.getInitializer())); - JType fieldType = field.getType().getUnderlyingType(); - assert fieldType instanceof JPrimitiveType || fieldType == program.getTypeJavaLangString() - : fieldType.getName() + " is not a primitive nor String "; - if (fieldType != value.getType()) { - value = new JCastOperation(value.getSourceInfo(), fieldType, value); - } - resolveValuesByField.put(field, value); } + // TODO(rluble): Simplify the expression to a literal here after refactoring Simplifier. + // The initializer is guaranteed to be constant but it may be a non literal expression, so + // clone it and recursively remove field references, and finally cache the results. + value = accept(new CloneExpressionVisitor().cloneExpression(field.getInitializer())); + JType fieldType = field.getType().getUnderlyingType(); + assert fieldType instanceof JPrimitiveType || fieldType == program.getTypeJavaLangString() + : fieldType.getName() + " is not a primitive nor String"; + if (fieldType != value.getType()) { + value = new JCastOperation(value.getSourceInfo(), fieldType, value); + } + resolveValuesByField.put(field, value); return value; } - public CompileTimeConstantsReplacingVisitor(JProgram program) { + private CompileTimeConstantsReplacingVisitor(JProgram program) { this.program = program; } } diff --git a/dev/core/src/com/google/gwt/util/tools/ArgHandlerEnum.java b/dev/core/src/com/google/gwt/util/tools/ArgHandlerEnum.java index e74d6a6ea83..f652d87e0ed 100644 --- a/dev/core/src/com/google/gwt/util/tools/ArgHandlerEnum.java +++ b/dev/core/src/com/google/gwt/util/tools/ArgHandlerEnum.java @@ -15,16 +15,19 @@ */ package com.google.gwt.util.tools; -import com.google.gwt.thirdparty.guava.common.base.Function; +import static com.google.gwt.thirdparty.guava.common.base.Preconditions.checkNotNull; + +import com.google.gwt.thirdparty.guava.common.base.Ascii; +import com.google.gwt.thirdparty.guava.common.base.Enums; import com.google.gwt.thirdparty.guava.common.base.Joiner; +import com.google.gwt.thirdparty.guava.common.base.Preconditions; import com.google.gwt.thirdparty.guava.common.base.Predicate; -import com.google.gwt.thirdparty.guava.common.collect.Collections2; -import com.google.gwt.thirdparty.guava.common.collect.Lists; +import com.google.gwt.thirdparty.guava.common.collect.FluentIterable; import java.util.Arrays; -import java.util.Collection; import java.util.List; -import java.util.Locale; + +import javax.annotation.Nullable; /** * A generic arg handler for options defined by enums. @@ -39,7 +42,7 @@ public abstract class ArgHandlerEnum> extends ArgHandler { private static final int ABBREVIATION_MIN_SIZE = 3; /** - * Constructor, assumes that the default value for the options is the first enum value. + * Constructor, default value must be handled by the user code. */ public ArgHandlerEnum(Class optionsEnumClass) { this(optionsEnumClass, null, false); @@ -48,9 +51,14 @@ public ArgHandlerEnum(Class optionsEnumClass) { /** * Constructor, allows to specify the default value for the option and whether to accept or * prefixes as abbreviations. + *

+ * When {@code defaultValue} is null, handling of the default for the option is left to be + * handled by the user code. */ - public ArgHandlerEnum(Class optionsEnumClass, T defaultValue, boolean allowAbbreviation) { - this.optionsEnumClass = optionsEnumClass; + public ArgHandlerEnum( + Class optionsEnumClass, @Nullable T defaultValue, boolean allowAbbreviation) { + Preconditions.checkArgument(optionsEnumClass.getEnumConstants().length > 1); + this.optionsEnumClass = checkNotNull(optionsEnumClass); this.defaultValue = defaultValue; this.allowAbbreviation = allowAbbreviation; } @@ -70,29 +78,34 @@ public String[] getTagArgs() { @Override public final int handle(String[] args, int startIndex) { - if (startIndex + 1 < args.length) { - String value = args[startIndex + 1].trim().toUpperCase(Locale.ROOT); + // A command line argument corresponding to an enum option has a parameter + // (the desired value), will be at startIndex + 1. + int optionValueIndex = startIndex + 1; + if (optionValueIndex < args.length) { + String value = Ascii.toUpperCase(args[optionValueIndex].trim()); T mode = matchOption(value); if (mode == null) { - System.err.println(value + " is not a valid option for " + getTag()); - System.err.println( - getTag() + " value must be one of " + - getFormattedOptionNames(", ", " or ", optionsEnumClass) + "."); + System.err.format("%s is not a valid option for %s\n", value, getTag()); + System.err.format("%s value must be one of %s.\n", + getTag(), getFormattedOptionNames(", ", " or ", optionsEnumClass)); return -1; } setValue(mode); return 1; } - System.err.println("Missing argument for " + getTag() + " must be followed by one of " + - getFormattedOptionNames(", ", " or ", optionsEnumClass) + "."); + System.err.format("Missing argument for %s must be followed by one of %s.\n" + + getTag(), getFormattedOptionNames(", ", " or ", optionsEnumClass)); return -1; } protected String getPurposeString(String prefix) { - return (isExperimental() ? "EXPERIMENTAL: " : "") + prefix + " " + - getFormattedOptionNames(", ", " or ", optionsEnumClass) + - (defaultValue != null ? " (defaults to " + defaultValue.name() + ")" : ""); + String maybeExperimentalString = isExperimental() ? "EXPERIMENTAL: " : ""; + String defaultValueString = defaultValue != null ? + " (defaults to " + defaultValue.name() + ")" : ""; + + return String.format("%s%s %s%s", maybeExperimentalString, prefix, + getFormattedOptionNames(", ", " or ", optionsEnumClass), defaultValueString); } /** @@ -101,46 +114,41 @@ protected String getPurposeString(String prefix) { public abstract void setValue(T value); private List getEnumNames(Class optionsEnumClass) { - return Lists.transform(Arrays.asList(optionsEnumClass.getEnumConstants()), - new Function() { - @Override - public String apply(T t) { - return t.name(); - } - }); + return FluentIterable.from(Arrays.asList(optionsEnumClass.getEnumConstants())) + .transform(Enums.stringConverter(optionsEnumClass).reverse()) + .toList(); } - private String getFormattedOptionNames(String separator,Class optionsEnumClass) { + private String getFormattedOptionNames(String separator, Class optionsEnumClass) { return getFormattedOptionNames(separator, separator, optionsEnumClass); } - private String getFormattedOptionNames(String separator,String lastSeparator, - Class optionsEnumClass) { + private String getFormattedOptionNames( + String separator, String lastSeparator, Class optionsEnumClass) { List enumNames = getEnumNames(optionsEnumClass); List allNamesButLast = enumNames.subList(0, enumNames.size() - 1); String lastName = enumNames.get(enumNames.size() - 1); - return Joiner.on(lastSeparator).join(Joiner.on(separator).join(allNamesButLast), lastName); + return Joiner.on(separator).join(allNamesButLast) + lastSeparator + lastName; } - private T matchOption(final String value) { - try { - Collection matches = Collections2.filter( - Arrays.asList(optionsEnumClass.getEnumConstants()), - new Predicate() { - @Override - public boolean apply(T t) { - if (allowAbbreviation && value.length() >= ABBREVIATION_MIN_SIZE) { - return t.name().startsWith(value); - } - return t.name().equals(value); - } - }); - if (matches.size() == 1) { - return matches.iterator().next(); - } - } catch (IllegalArgumentException e) { - } - return null; + private Predicate> buildMatchPredicate(final String value) { + return + new Predicate>() { + @Override + public boolean apply(Enum t) { + if (allowAbbreviation && value.length() >= ABBREVIATION_MIN_SIZE) { + return t.name().startsWith(value); + } + return t.name().equals(value); + } + }; + } + + private T matchOption(String value) { + List matchedOptions = FluentIterable.from(Arrays.asList(optionsEnumClass.getEnumConstants())) + .filter(buildMatchPredicate(value)) + .toList(); + return matchedOptions.size() == 1 ? matchedOptions.get(0) : null; } } diff --git a/dev/core/test/com/google/gwt/util/tools/ArgHandlerEnumTest.java b/dev/core/test/com/google/gwt/util/tools/ArgHandlerEnumTest.java index f7b3f20d062..0912a2ca851 100644 --- a/dev/core/test/com/google/gwt/util/tools/ArgHandlerEnumTest.java +++ b/dev/core/test/com/google/gwt/util/tools/ArgHandlerEnumTest.java @@ -24,17 +24,21 @@ public class ArgHandlerEnumTest extends TestCase { enum SomeFlags { THIS_FLAG, THAT_FLAG, THAT_OTHER_FLAG, ANOTHER_FLAG } - static public SomeFlags optionValue = null; + enum EmptyEnum { } - private abstract class MockArgHandlerEnumBase extends ArgHandlerEnum { + enum OneOptionEnum { SINGLE_OPTION } - public MockArgHandlerEnumBase() { - super(SomeFlags.class); + static public Enum optionValue = null; + + private abstract class MockArgHandlerEnumBase> extends ArgHandlerEnum { + + public MockArgHandlerEnumBase(Class enumClass) { + super(enumClass); } - public MockArgHandlerEnumBase(SomeFlags defaultValue, - boolean allowAbbraviation) { - super(SomeFlags.class, defaultValue, allowAbbraviation); + public MockArgHandlerEnumBase( + Class enumClass, T defaultValue, boolean allowAbbraviation) { + super(enumClass, defaultValue, allowAbbraviation); } @Override @@ -53,13 +57,13 @@ public boolean isExperimental() { } @Override - public void setValue(SomeFlags value) { + public void setValue(T value) { optionValue = value; } } public void testHandle() { - ArgHandler handler = new MockArgHandlerEnumBase() { }; + ArgHandler handler = new MockArgHandlerEnumBase(SomeFlags.class) { }; optionValue = null; int consuemdArguments = handler.handle(new String[] {"-Xflag", "THIS_FLAG"}, 0); assertEquals(1, consuemdArguments); @@ -79,7 +83,8 @@ public void testHandle() { } public void testHandle_default() { - ArgHandler handler = new MockArgHandlerEnumBase(SomeFlags.THAT_OTHER_FLAG, false) { }; + ArgHandler handler = new MockArgHandlerEnumBase( + SomeFlags.class, SomeFlags.THAT_OTHER_FLAG, false) { }; optionValue = null; int consuemdArguments = handler.handle(handler.getDefaultArgs(), 0); assertEquals(1, consuemdArguments); @@ -87,7 +92,7 @@ public void testHandle_default() { } public void testHandle_Abbreviations() { - ArgHandler handler = new MockArgHandlerEnumBase(null, true) { }; + ArgHandler handler = new MockArgHandlerEnumBase(SomeFlags.class, null, true) { }; optionValue = null; int consuemdArguments = handler.handle(new String[] {"-Xflag", "THIS"}, 0); assertEquals(1, consuemdArguments); @@ -115,13 +120,32 @@ public void testHandle_Abbreviations() { } public void testGetDefaultTags() { - ArgHandler handler = new MockArgHandlerEnumBase() { }; + ArgHandler handler = new MockArgHandlerEnumBase(SomeFlags.class) { }; assertNull(handler.getDefaultArgs()); - handler = new MockArgHandlerEnumBase(SomeFlags.THAT_FLAG, false) { }; + handler = + new MockArgHandlerEnumBase(SomeFlags.class, SomeFlags.THAT_FLAG, false) { }; assertContentsEquals(new String[]{"-Xflag", "THAT_FLAG"}, handler.getDefaultArgs()); } + public void testGetPurposeString() { + assertEquals("EXPERIMENTAL: Set flag: THIS_FLAG, THAT_FLAG, THAT_OTHER_FLAG or ANOTHER_FLAG", + new MockArgHandlerEnumBase(SomeFlags.class) { }.getPurpose()); + } + + public void testBadEnums() { + try { + new MockArgHandlerEnumBase(EmptyEnum.class) { }; + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + } + + try { + new MockArgHandlerEnumBase(OneOptionEnum.class) { }; + fail("Should have thrown IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + } + } private void assertContentsEquals(T[] expected, T[] actual) { assertEquals("Different sizes", expected.length, actual.length); for (int i = 0; i < expected.length; i++) {