From d1b34d487bb83f0761d707cb8b27f88d547068e8 Mon Sep 17 00:00:00 2001 From: brandjon Date: Tue, 18 Apr 2017 15:52:57 +0200 Subject: Eliminate some middleman methods Reduce spaghetti code by exposing the parser's OptionsData as package-private, rather than exposing individual methods ad hoc between OptionsParser and OptionsParserImpl. Also change some calls from static constructors to diamond syntax. RELNOTES: None PiperOrigin-RevId: 153457442 --- .../devtools/common/options/OptionsParser.java | 60 ++++++++++++---------- .../devtools/common/options/OptionsParserImpl.java | 34 +++++------- .../devtools/common/options/OptionsUsage.java | 6 +-- 3 files changed, 49 insertions(+), 51 deletions(-) diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java index 3b55c744c0..9574a90162 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -20,16 +20,14 @@ import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; -import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import com.google.common.escape.Escaper; import java.lang.reflect.Field; import java.nio.file.FileSystem; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -93,7 +91,7 @@ public class OptionsParser implements OptionsProvider { * options classes on the classpath. */ private static final Map>, OptionsData> optionsData = - Maps.newHashMap(); + new HashMap<>(); /** * Returns {@link OpaqueOptionsData} suitable for passing along to {@link @@ -104,32 +102,35 @@ public class OptionsParser implements OptionsProvider { * construct lots of different {@link OptionsParser} instances). */ public static OpaqueOptionsData getOptionsData( - ImmutableList> optionsClasses) throws ConstructionException { + List> optionsClasses) throws ConstructionException { return getOptionsDataInternal(optionsClasses); } - private static synchronized OptionsData getOptionsDataInternal( - ImmutableList> optionsClasses) throws ConstructionException { - OptionsData result = optionsData.get(optionsClasses); + /** + * Returns the {@link OptionsData} associated with the given list of options classes. + */ + static synchronized OptionsData getOptionsDataInternal( + List> optionsClasses) throws ConstructionException { + ImmutableList> immutableOptionsClasses = + ImmutableList.copyOf(optionsClasses); + OptionsData result = optionsData.get(immutableOptionsClasses); if (result == null) { try { - result = OptionsData.from(optionsClasses); + result = OptionsData.from(immutableOptionsClasses); } catch (Exception e) { throw new ConstructionException(e.getMessage(), e); } - optionsData.put(optionsClasses, result); + optionsData.put(immutableOptionsClasses, result); } return result; } /** - * Returns all the annotated fields for the given class, including inherited - * ones. + * Returns the {@link OptionsData} associated with the given options class. */ - static Collection getAllAnnotatedFields(Class optionsClass) { - OptionsData data = getOptionsDataInternal( - ImmutableList.>of(optionsClass)); - return data.getFieldsForClass(optionsClass); + static OptionsData getOptionsDataInternal(Class optionsClass) + throws ConstructionException { + return getOptionsDataInternal(ImmutableList.>of(optionsClass)); } /** @@ -328,7 +329,7 @@ public class OptionsParser implements OptionsProvider { // The generic type of the list is not known at runtime, so we can't // use it here. It was already checked in the constructor, so this is // type-safe. - List result = Lists.newArrayList(); + List result = new ArrayList<>(); ListMultimap realValue = (ListMultimap) value; for (OptionPriority priority : OptionPriority.values()) { // If there is no mapping for this key, this check avoids object creation (because @@ -536,11 +537,12 @@ public class OptionsParser implements OptionsProvider { */ public String describeOptions( Map categoryDescriptions, HelpVerbosity helpVerbosity) { + OptionsData data = impl.getOptionsData(); StringBuilder desc = new StringBuilder(); - if (!impl.getOptionsClasses().isEmpty()) { - List allFields = Lists.newArrayList(); - for (Class optionsClass : impl.getOptionsClasses()) { - allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass)); + if (!data.getOptionsClasses().isEmpty()) { + List allFields = new ArrayList<>(); + for (Class optionsClass : data.getOptionsClasses()) { + allFields.addAll(data.getFieldsForClass(optionsClass)); } Collections.sort(allFields, OptionsUsage.BY_CATEGORY); String prevCategory = null; @@ -579,11 +581,12 @@ public class OptionsParser implements OptionsProvider { * of the category. */ public String describeOptionsHtml(Map categoryDescriptions, Escaper escaper) { + OptionsData data = impl.getOptionsData(); StringBuilder desc = new StringBuilder(); - if (!impl.getOptionsClasses().isEmpty()) { - List allFields = Lists.newArrayList(); - for (Class optionsClass : impl.getOptionsClasses()) { - allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass)); + if (!data.getOptionsClasses().isEmpty()) { + List allFields = new ArrayList<>(); + for (Class optionsClass : data.getOptionsClasses()) { + allFields.addAll(data.getFieldsForClass(optionsClass)); } Collections.sort(allFields, OptionsUsage.BY_CATEGORY); String prevCategory = null; @@ -620,12 +623,13 @@ public class OptionsParser implements OptionsProvider { * details on the format for the flag completion. */ public String getOptionsCompletion() { + OptionsData data = impl.getOptionsData(); StringBuilder desc = new StringBuilder(); // List all options - List allFields = Lists.newArrayList(); - for (Class optionsClass : impl.getOptionsClasses()) { - allFields.addAll(impl.getAnnotatedFieldsFor(optionsClass)); + List allFields = new ArrayList<>(); + for (Class optionsClass : data.getOptionsClasses()) { + allFields.addAll(data.getFieldsForClass(optionsClass)); } // Sort field for deterministic ordering Collections.sort(allFields, new Comparator() { 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 918fef1b8d..5c635fb524 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -25,7 +25,6 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Iterators; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.devtools.common.options.OptionsParser.OptionDescription; import com.google.devtools.common.options.OptionsParser.OptionUsageRestrictions; @@ -33,10 +32,11 @@ import com.google.devtools.common.options.OptionsParser.OptionValueDescription; import com.google.devtools.common.options.OptionsParser.UnparsedOptionValueDescription; import java.lang.reflect.Constructor; import java.lang.reflect.Field; +import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; @@ -61,14 +61,14 @@ class OptionsParserImpl { * * This map is modified by repeated calls to {@link #parse(OptionPriority,Function,List)}. */ - private final Map parsedValues = Maps.newHashMap(); + private final Map parsedValues = new HashMap<>(); /** * We store the pre-parsed, explicit options for each priority in here. * We use partially preparsed options, which can be different from the original * representation, e.g. "--nofoo" becomes "--foo=0". */ - private final List unparsedValues = Lists.newArrayList(); + private final List unparsedValues = new ArrayList<>(); /** * Unparsed values for use with the canonicalize command are stored separately from @@ -82,7 +82,7 @@ class OptionsParserImpl { private final Multimap canonicalizeValues = LinkedHashMultimap.create(); - private final List warnings = Lists.newArrayList(); + private final List warnings = new ArrayList<>(); private boolean allowSingleDashLongOptions = false; @@ -122,8 +122,10 @@ class OptionsParserImpl { * The implementation of {@link OptionsBase#asMap}. */ static Map optionsAsMap(OptionsBase optionsInstance) { - Map map = Maps.newHashMap(); - for (Field field : OptionsParser.getAllAnnotatedFields(optionsInstance.getClass())) { + Class optionsClass = optionsInstance.getClass(); + OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); + Map map = new HashMap<>(); + for (Field field : data.getFieldsForClass(optionsClass)) { try { String name = field.getAnnotation(Option.class).name(); Object value = field.get(optionsInstance); @@ -135,10 +137,6 @@ class OptionsParserImpl { return map; } - List getAnnotatedFieldsFor(Class clazz) { - return optionsData.getFieldsForClass(clazz); - } - /** * Implements {@link OptionsParser#asListOfUnparsedOptions()}. */ @@ -200,7 +198,7 @@ class OptionsParserImpl { } }); - List result = Lists.newArrayList(); + List result = new ArrayList<>(); for (UnparsedOptionValueDescription value : processed) { // Ignore expansion options. @@ -217,7 +215,7 @@ class OptionsParserImpl { * Implements {@link OptionsParser#asListOfEffectiveOptions()}. */ List asListOfEffectiveOptions() { - List result = Lists.newArrayList(); + List result = new ArrayList<>(); for (Map.Entry mapEntry : optionsData.getAllNamedFields()) { String fieldName = mapEntry.getKey(); Field field = mapEntry.getValue(); @@ -241,10 +239,6 @@ class OptionsParserImpl { return result; } - Collection> getOptionsClasses() { - return optionsData.getOptionsClasses(); - } - private void maybeAddDeprecationWarning(Field field) { Option option = field.getAnnotation(Option.class); // Continue to support the old behavior for @Deprecated options. @@ -449,8 +443,8 @@ class OptionsParserImpl { String expandedFrom, List args) throws OptionsParsingException { - List unparsedArgs = Lists.newArrayList(); - LinkedHashMap> implicitRequirements = Maps.newLinkedHashMap(); + List unparsedArgs = new ArrayList<>(); + LinkedHashMap> implicitRequirements = new LinkedHashMap<>(); Iterator argsIterator = argsPreProcessor.preProcess(args).iterator(); while (argsIterator.hasNext()) { @@ -712,7 +706,7 @@ class OptionsParserImpl { if (constructor == null) { return null; } - optionsInstance = constructor.newInstance(new Object[0]); + optionsInstance = constructor.newInstance(); } catch (Exception e) { throw new IllegalStateException(e); } 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 b898ae6c28..1c9f5193f2 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsUsage.java +++ b/src/main/java/com/google/devtools/common/options/OptionsUsage.java @@ -16,10 +16,10 @@ 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.Lists; import com.google.common.escape.Escaper; import java.lang.reflect.Field; import java.text.BreakIterator; +import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -40,8 +40,8 @@ class OptionsUsage { * OptionsBase} subclasses they depend on until a complete parser is constructed). */ static void getUsage(Class optionsClass, StringBuilder usage) { - List optionFields = - Lists.newArrayList(OptionsParser.getAllAnnotatedFields(optionsClass)); + OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); + List optionFields = new ArrayList<>(data.getFieldsForClass(optionsClass)); Collections.sort(optionFields, BY_NAME); for (Field optionField : optionFields) { getUsage(optionField, usage, OptionsParser.HelpVerbosity.LONG, null); -- cgit v1.2.3