Use Object for what instead of String
The string is only used for error messaging on failure. Instead of constructing
lots of strings allow passing in a generic Object whos toString is used for the
error message. This allows us to bypass a lot of garbage string generation, at
th expense of a little indirection.
Chose Object over some specialized type, since we use inline strings frequently
as well.
--
MOS_MIGRATED_REVID=133741724
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
index 3a8c69c..825efb5 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -28,7 +28,6 @@
import com.google.devtools.build.lib.syntax.Type.ConversionException;
import com.google.devtools.build.lib.syntax.Type.DictType;
import com.google.devtools.build.lib.syntax.Type.ListType;
-
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
@@ -36,7 +35,6 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
-
import javax.annotation.Nullable;
/**
@@ -87,7 +85,7 @@
}
@Override
- public DistributionType convert(Object x, String what, Object context) {
+ public DistributionType convert(Object x, Object what, Object context) {
throw new UnsupportedOperationException();
}
@@ -158,7 +156,7 @@
* <p>The caller is responsible for casting the returned value appropriately.
*/
public static <T> Object selectableConvert(
- Type type, Object x, String what, @Nullable Label context)
+ Type type, Object x, Object what, @Nullable Label context)
throws ConversionException {
if (x instanceof com.google.devtools.build.lib.syntax.SelectorList) {
return new SelectorList<T>(
@@ -177,7 +175,7 @@
}
@Override
- public FilesetEntry convert(Object x, String what, Object context)
+ public FilesetEntry convert(Object x, Object what, Object context)
throws ConversionException {
if (!(x instanceof FilesetEntry)) {
throw new ConversionException(this, x, what);
@@ -233,7 +231,7 @@
}
@Override
- public Label convert(Object x, String what, Object context)
+ public Label convert(Object x, Object what, Object context)
throws ConversionException {
if (x instanceof Label) {
return (Label) x;
@@ -259,7 +257,7 @@
}
@Override
- public License convert(Object x, String what, Object context) throws ConversionException {
+ public License convert(Object x, Object what, Object context) throws ConversionException {
try {
List<String> licenseStrings = STRING_LIST.convert(x, what);
return License.parseLicense(licenseStrings);
@@ -303,7 +301,7 @@
}
@Override
- public Set<DistributionType> convert(Object x, String what, Object context)
+ public Set<DistributionType> convert(Object x, Object what, Object context)
throws ConversionException {
try {
List<String> distribStrings = STRING_LIST.convert(x, what);
@@ -366,7 +364,7 @@
}
@Override
- public Label convert(Object x, String what, Object context)
+ public Label convert(Object x, Object what, Object context)
throws ConversionException {
String value;
@@ -401,7 +399,7 @@
private final List<Selector<T>> elements;
@VisibleForTesting
- SelectorList(List<Object> x, String what, @Nullable Label context,
+ SelectorList(List<Object> x, Object what, @Nullable Label context,
Type<T> originalType) throws ConversionException {
if (x.size() > 1 && originalType.concat(ImmutableList.<T>of()) == null) {
throw new ConversionException(
@@ -411,7 +409,7 @@
ImmutableList.Builder<Selector<T>> builder = ImmutableList.builder();
for (Object elem : x) {
if (elem instanceof SelectorValue) {
- builder.add(new Selector<T>(((SelectorValue) elem).getDictionary(), what,
+ builder.add(new Selector<>(((SelectorValue) elem).getDictionary(), what,
context, originalType, ((SelectorValue) elem).getNoMatchError()));
} else {
T directValue = originalType.convert(elem, what, context);
@@ -482,7 +480,7 @@
/**
* Creates a new Selector using the default error message when no conditions match.
*/
- Selector(ImmutableMap<?, ?> x, String what, @Nullable Label context, Type<T> originalType)
+ Selector(ImmutableMap<?, ?> x, Object what, @Nullable Label context, Type<T> originalType)
throws ConversionException {
this(x, what, context, originalType, "");
}
@@ -490,7 +488,7 @@
/**
* Creates a new Selector with a custom error message for when no conditions match.
*/
- Selector(ImmutableMap<?, ?> x, String what, @Nullable Label context, Type<T> originalType,
+ Selector(ImmutableMap<?, ?> x, Object what, @Nullable Label context, Type<T> originalType,
String noMatchError) throws ConversionException {
this.originalType = originalType;
LinkedHashMap<Label, T> result = new LinkedHashMap<>();
@@ -633,7 +631,7 @@
// Like BooleanType, this must handle integers as well.
@Override
- public TriState convert(Object x, String what, Object context)
+ public TriState convert(Object x, Object what, Object context)
throws ConversionException {
if (x instanceof TriState) {
return (TriState) x;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index cba10a7..53aabfa 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1727,9 +1727,11 @@
*/
private static Object convertFromBuildLangType(Rule rule, Attribute attr, Object buildLangValue)
throws ConversionException {
- String what = "attribute '" + attr.getName() + "' in '" + rule.getRuleClass() + "' rule";
- Object converted =
- BuildType.selectableConvert(attr.getType(), buildLangValue, what, rule.getLabel());
+ Object converted = BuildType.selectableConvert(
+ attr.getType(),
+ buildLangValue,
+ new AttributeConversionContext(attr.getName(), rule.getRuleClass()),
+ rule.getLabel());
if ((converted instanceof SelectorList<?>) && !attr.isConfigurable()) {
throw new ConversionException(
@@ -1749,6 +1751,28 @@
}
/**
+ * Provides a {@link #toString()} description of the attribute being converted for
+ * {@link BuildType#selectableConvert}. This is preferred over a raw string to avoid uselessly
+ * constructing strings which are never used. A separate class instead of inline to avoid
+ * accidental memory leaks.
+ */
+ private static class AttributeConversionContext {
+ private final String attrName;
+ private final String ruleClass;
+
+ AttributeConversionContext(String attrName, String ruleClass) {
+ this.attrName = attrName;
+ this.ruleClass = ruleClass;
+ }
+
+ @Override
+ public String toString() {
+ return "attribute '" + attrName + "' in '" + ruleClass + "' rule";
+ }
+ }
+
+
+ /**
* Verifies that the rule has a valid value for the attribute according to its allowed values.
*
* <p>If the value for the given attribute on the given rule is invalid, an error will be recorded
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index a8987e8..37c280c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -23,7 +23,6 @@
import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
import com.google.devtools.build.lib.util.LoggingUtil;
import com.google.devtools.build.lib.util.StringCanonicalizer;
-
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedHashSet;
@@ -33,7 +32,6 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.logging.Level;
-
import javax.annotation.Nullable;
/**
@@ -73,14 +71,14 @@
* {@link com.google.devtools.build.lib.packages.BuildType#selectableConvert}.
*
* @param x the build-interpreter value to convert.
- * @param what a string description of what x is for; should be included in
- * any exception thrown. Grammatically, must describe a syntactic
+ * @param what an object having a toString describing what x is for; should be included in
+ * any exception thrown. Grammatically, must produce a string describe a syntactic
* construct, e.g. "attribute 'srcs' of rule foo".
* @param context the label of the current BUILD rule; must be non-null if resolution of
* package-relative label strings is required
* @throws ConversionException if there was a problem performing the type conversion
*/
- public abstract T convert(Object x, String what, @Nullable Object context)
+ public abstract T convert(Object x, Object what, @Nullable Object context)
throws ConversionException;
// TODO(bazel-team): Check external calls (e.g. in PackageFactory), verify they always want
// this over selectableConvert.
@@ -90,7 +88,7 @@
* Useful for converting values to types that do not involve the type {@code LABEL}
* and hence do not require the label of the current package.
*/
- public final T convert(Object x, String what) throws ConversionException {
+ public final T convert(Object x, Object what) throws ConversionException {
return convert(x, what, null);
}
@@ -240,7 +238,7 @@
* an explanatory error message.
*/
public static class ConversionException extends EvalException {
- private static String message(Type<?> type, Object value, String what) {
+ private static String message(Type<?> type, Object value, @Nullable Object what) {
StringBuilder builder = new StringBuilder();
builder.append("expected value of type '").append(type).append("'");
if (what != null) {
@@ -252,7 +250,7 @@
return builder.toString();
}
- public ConversionException(Type<?> type, Object value, String what) {
+ public ConversionException(Type<?> type, Object value, @Nullable Object what) {
super(null, message(type, value, what));
}
@@ -295,7 +293,7 @@
}
@Override
- public Object convert(Object x, String what, Object context) {
+ public Object convert(Object x, Object what, Object context) {
return x;
}
}
@@ -327,7 +325,7 @@
}
@Override
- public Integer convert(Object x, String what, Object context)
+ public Integer convert(Object x, Object what, Object context)
throws ConversionException {
if (!(x instanceof Integer)) {
throw new ConversionException(this, x, what);
@@ -373,7 +371,7 @@
// Conversion to boolean must also tolerate integers of 0 and 1 only.
@Override
- public Boolean convert(Object x, String what, Object context)
+ public Boolean convert(Object x, Object what, Object context)
throws ConversionException {
if (x instanceof Boolean) {
return (Boolean) x;
@@ -428,7 +426,7 @@
}
@Override
- public String convert(Object x, String what, Object context)
+ public String convert(Object x, Object what, Object context)
throws ConversionException {
if (!(x instanceof String)) {
throw new ConversionException(this, x, what);
@@ -512,11 +510,11 @@
}
@Override
- public Map<KeyT, ValueT> convert(Object x, String what, Object context)
+ public Map<KeyT, ValueT> convert(Object x, Object what, Object context)
throws ConversionException {
if (!(x instanceof Map<?, ?>)) {
throw new ConversionException(String.format(
- "Expected a map for dictionary but got a %s", x.getClass().getName()));
+ "Expected a map for dictionary but got a %s", x.getClass().getName()));
}
// Order the keys so the return value will be independent of insertion order.
Map<KeyT, ValueT> result = new TreeMap<>();
@@ -596,7 +594,7 @@
}
@Override
- public List<ElemT> convert(Object x, String what, Object context)
+ public List<ElemT> convert(Object x, Object what, Object context)
throws ConversionException {
if (!(x instanceof Iterable<?>)) {
throw new ConversionException(this, x, what);
@@ -604,8 +602,10 @@
int index = 0;
Iterable<?> iterable = (Iterable<?>) x;
List<ElemT> result = new ArrayList<>(Iterables.size(iterable));
+ ListConversionContext conversionContext = new ListConversionContext(what);
for (Object elem : iterable) {
- ElemT converted = elemType.convert(elem, "element " + index + " of " + what, context);
+ conversionContext.update(index);
+ ElemT converted = elemType.convert(elem, conversionContext, context);
if (converted != null) {
result.add(converted);
} else {
@@ -660,6 +660,30 @@
}
return tags;
}
+
+ /**
+ * Provides a {@link #toString()} description of the context of the value in a list being
+ * converted. This is preferred over a raw string to avoid uselessly constructing strings which
+ * are never used. This class is mutable (the index is updated).
+ */
+ private static class ListConversionContext {
+ private final Object what;
+ private int index = 0;
+
+ ListConversionContext(Object what) {
+ this.what = what;
+ }
+
+ void update(int index) {
+ this.index = index;
+ }
+
+ @Override
+ public String toString() {
+ return "element " + index + " of " + what;
+ }
+
+ }
}
/** Type for lists of arbitrary objects */
@@ -673,7 +697,7 @@
@Override
@SuppressWarnings("unchecked")
- public List<Object> convert(Object x, String what, Object context)
+ public List<Object> convert(Object x, Object what, Object context)
throws ConversionException {
if (x instanceof SkylarkList) {
return ((SkylarkList) x).getImmutableList();
