Skip to content

Commit

Permalink
Merge pull request #242 from google/issue/240.merge.nans
Browse files Browse the repository at this point in the history
Fix treatment of NaN defaults in mergeFrom
  • Loading branch information
alicederyn committed Feb 6, 2017
2 parents ac19c7e + 0393909 commit 1122b87
Show file tree
Hide file tree
Showing 12 changed files with 459 additions and 171 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import static org.inferred.freebuilder.processor.Metadata.GET_CODE_GENERATOR;
import static org.inferred.freebuilder.processor.Metadata.UnderrideLevel.ABSENT;
import static org.inferred.freebuilder.processor.Metadata.UnderrideLevel.FINAL;
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NOT_NULLABLE;
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NULLABLE;
import static org.inferred.freebuilder.processor.util.feature.GuavaLibrary.GUAVA;
import static org.inferred.freebuilder.processor.util.feature.SourceLevel.SOURCE_LEVEL;

Expand All @@ -41,6 +43,7 @@
import org.inferred.freebuilder.processor.util.Block;
import org.inferred.freebuilder.processor.util.Excerpt;
import org.inferred.freebuilder.processor.util.Excerpts;
import org.inferred.freebuilder.processor.util.ObjectsExcerpts;
import org.inferred.freebuilder.processor.util.PreconditionExcerpts;
import org.inferred.freebuilder.processor.util.SourceBuilder;

Expand Down Expand Up @@ -395,26 +398,12 @@ private static void addValueTypeEquals(SourceBuilder code, Metadata metadata) {
code.add(";\n");
} else {
for (Property property : metadata.getProperties()) {
switch (property.getType().getKind()) {
case FLOAT:
case DOUBLE:
code.addLine(" if (%s.doubleToLongBits(%s)", Double.class, property.getName())
.addLine(" != %s.doubleToLongBits(other.%s)) {",
Double.class, property.getName());
break;

default:
if (property.getType().getKind().isPrimitive()) {
code.addLine(" if (%1$s != other.%1$s) {", property.getName());
} else if (property.getCodeGenerator().getType() == Type.OPTIONAL) {
code.addLine(" if (%1$s != other.%1$s", property.getName())
.addLine(" && (%1$s == null || !%1$s.equals(other.%1$s))) {",
property.getName());
} else {
code.addLine(" if (!%1$s.equals(other.%1$s)) {", property.getName());
}
}
code.addLine(" return false;")
code.addLine(" if (%s) {", ObjectsExcerpts.notEquals(
property.getName(),
"other." + property.getName(),
property.getType().getKind(),
(property.getCodeGenerator().getType() == Type.OPTIONAL) ? NULLABLE : NOT_NULLABLE))
.addLine(" return false;")
.addLine(" }");
}
code.addLine(" return true;");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.inferred.freebuilder.processor.BuilderMethods.getter;
import static org.inferred.freebuilder.processor.BuilderMethods.mapper;
import static org.inferred.freebuilder.processor.BuilderMethods.setter;
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NOT_NULLABLE;
import static org.inferred.freebuilder.processor.util.PreconditionExcerpts.checkNotNullInline;
import static org.inferred.freebuilder.processor.util.PreconditionExcerpts.checkNotNullPreamble;
import static org.inferred.freebuilder.processor.util.feature.FunctionPackage.FUNCTION_PACKAGE;
Expand All @@ -31,10 +32,12 @@
import org.inferred.freebuilder.processor.util.Block;
import org.inferred.freebuilder.processor.util.Excerpt;
import org.inferred.freebuilder.processor.util.Excerpts;
import org.inferred.freebuilder.processor.util.ObjectsExcerpts;
import org.inferred.freebuilder.processor.util.ParameterizedType;
import org.inferred.freebuilder.processor.util.PreconditionExcerpts;
import org.inferred.freebuilder.processor.util.SourceBuilder;

import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;

/** Default {@link PropertyCodeGenerator.Factory}, providing reference semantics for any type. */
Expand All @@ -50,12 +53,12 @@ public Optional<? extends PropertyCodeGenerator> create(Config config) {
@VisibleForTesting static class CodeGenerator extends PropertyCodeGenerator {

private final boolean hasDefault;
private final boolean isPrimitive;
private final TypeKind kind;

CodeGenerator(Metadata metadata, Property property, boolean hasDefault) {
super(metadata, property);
this.hasDefault = hasDefault;
this.isPrimitive = property.getType().getKind().isPrimitive();
this.kind = property.getType().getKind();
}

@Override
Expand All @@ -82,14 +85,14 @@ private void addSetter(SourceBuilder code, final Metadata metadata) {
metadata.getType().javadocNoArgMethodLink(property.getGetterName()))
.addLine(" *")
.addLine(" * @return this {@code %s} object", metadata.getBuilder().getSimpleName());
if (!isPrimitive) {
if (!kind.isPrimitive()) {
code.addLine(" * @throws NullPointerException if {@code %s} is null", property.getName());
}
code.addLine(" */");
addAccessorAnnotations(code);
code.addLine("public %s %s(%s %s) {",
metadata.getBuilder(), setter(property), property.getType(), property.getName());
if (isPrimitive) {
if (kind.isPrimitive()) {
code.addLine(" this.%1$s = %1$s;", property.getName());
} else {
code.add(checkNotNullPreamble(property.getName()))
Expand Down Expand Up @@ -176,13 +179,11 @@ public void addMergeFromValue(Block code, String value) {
code.add("%s._unsetProperties.contains(%s.%s) || ",
defaults, metadata.getPropertyEnum(), property.getAllCapsName());
}
if (isPrimitive) {
code.add("%s.%s() != %s.%s()",
value, property.getGetterName(), defaults, getter(property));
} else {
code.add("!%s.%s().equals(%s.%s())",
value, property.getGetterName(), defaults, getter(property));
}
code.add(ObjectsExcerpts.notEquals(
Excerpts.add("%s.%s()", value, property.getGetterName()),
Excerpts.add("%s.%s()", defaults, getter(property)),
kind,
NOT_NULLABLE));
code.add(") {%n");
}
code.addLine(" %s(%s.%s());", setter(property), value, property.getGetterName());
Expand All @@ -204,11 +205,11 @@ public void addMergeFromBuilder(Block code, String builder) {
.add("(%s._unsetProperties.contains(%s.%s) ||",
defaults, metadata.getPropertyEnum(), property.getAllCapsName());
}
if (isPrimitive) {
code.add("%1$s.%2$s() != %3$s.%2$s()", builder, getter(property), defaults);
} else {
code.add("!%1$s.%2$s().equals(%3$s.%2$s())", builder, getter(property), defaults);
}
code.add(ObjectsExcerpts.notEquals(
Excerpts.add("%s.%s()", builder, getter(property)),
Excerpts.add("%s.%s()", defaults, getter(property)),
kind,
NOT_NULLABLE));
if (!hasDefault) {
code.add(")");
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package org.inferred.freebuilder.processor.util;

import static org.inferred.freebuilder.processor.util.feature.SourceLevel.SOURCE_LEVEL;

import javax.lang.model.type.TypeKind;

public class ObjectsExcerpts {

public enum Nullability {
NULLABLE, NOT_NULLABLE;

public boolean isNullable() {
return this == NULLABLE;
}
}

/**
* Returns an Excerpt equivalent to {@code Objects.equals(a, b)}.
*
* <p>If Objects is not available, {@code kind} and {@code nullability} are needed to generate
* the most idiomatic equivalent.
*/
public static Excerpt equals(Object a, Object b, TypeKind kind, Nullability nullability) {
return new EqualsExcerpt(true, a, b, kind, nullability);
}

/**
* Returns an Excerpt equivalent to {@code !Objects.equals(a, b)}.
*
* <p>If Objects is not available, {@code kind} and {@code nullability} are needed to generate
* the most idiomatic equivalent.
*/
public static Excerpt notEquals(Object a, Object b, TypeKind kind, Nullability nullability) {
return new EqualsExcerpt(false, a, b, kind, nullability);
}

private static class EqualsExcerpt extends Excerpt {

private final boolean areEqual;
private final Object a;
private final Object b;
private final TypeKind kind;
private final Nullability nullability;

EqualsExcerpt(boolean areEqual, Object a, Object b, TypeKind kind, Nullability nullability) {
this.areEqual = areEqual;
this.a = a;
this.b = b;
this.kind = kind;
this.nullability = nullability;
}

@Override
public void addTo(SourceBuilder code) {
QualifiedName javaUtilObjects = code.feature(SOURCE_LEVEL).javaUtilObjects().orNull();
if (javaUtilObjects != null) {
code.add("%s%s.equals(%s, %s)", areEqual ? "" : "!", javaUtilObjects, a, b);
} else if (nullability.isNullable()) {
if (areEqual) {
code.add("%1$s == %2$s || (%1$s != null && %1$s.equals(%2$s))", a, b);
} else {
code.add("%1$s != %2$s && (%1$s == null || !%1$s.equals(%2$s))", a, b);
}
} else if (kind.isPrimitive()) {
switch (kind) {
case FLOAT:
code.add("%1$s.floatToIntBits(%2$s) %3$s %1$s.floatToIntBits(%4$s)",
Float.class, a, areEqual ? "==" : "!=", b);
break;

case DOUBLE:
code.add("%1$s.doubleToLongBits(%2$s) %3$s %1$s.doubleToLongBits(%4$s)",
Double.class, a, areEqual ? "==" : "!=", b);
break;

default:
code.add("%s %s %s", a, areEqual ? "==" : "!=", b);
}
} else {
code.add("%s%s.equals(%s)", areEqual ? "" : "!", a, b);
}
}

@Override
protected void addFields(FieldReceiver fields) {
fields.add("areEqual", areEqual);
fields.add("a", a);
fields.add("b", b);
fields.add("kind", kind);
fields.add("nullable", nullability);
}

}



private ObjectsExcerpts() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ public static SourceBuilder simple(Feature<?>... features) {
new TypeShortener.AlwaysShorten(), new StaticFeatureSet(features));
}

/**
* Returns a {@link SourceStringBuilder} that returns compilable code.
*/
public static SourceBuilder compilable(FeatureSet features) {
return new SourceStringBuilder(
new TypeShortener.NeverShorten(), features);
}

SourceStringBuilder(TypeShortener shortener, FeatureSet features) {
this.shortener = shortener;
this.features = features;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,15 @@ public static Iterable<Object[]> featureSets() {
.addLine("public interface DataType {")
.addLine(" int propertyA();")
.addLine(" boolean propertyB();")
.addLine(" String propertyC();")
.addLine(" double propertyD();")
.addLine("")
.addLine(" class Builder extends DataType_Builder {")
.addLine(" public Builder() {")
.addLine(" propertyA(0);")
.addLine(" propertyB(false);")
.addLine(" propertyC(\"default\");")
.addLine(" propertyD(Double.NaN);")
.addLine(" }")
.addLine(" }")
.addLine("}")
Expand All @@ -82,12 +86,16 @@ public static Iterable<Object[]> featureSets() {
.addLine("public interface DataType {")
.addLine(" int propertyA();")
.addLine(" boolean propertyB();")
.addLine(" String propertyC();")
.addLine(" double propertyD();")
.addLine("")
.addLine(" class Builder extends DataType_Builder {")
.addLine(" public Builder() {")
.addLine(" if (true) { // Disable optimization in javac")
.addLine(" propertyA(0);")
.addLine(" propertyB(false);")
.addLine(" propertyC(\"default\");")
.addLine(" propertyD(Double.NaN);")
.addLine(" }")
.addLine(" }")
.addLine(" }")
Expand All @@ -108,10 +116,14 @@ public void testMergeFromBuilder_defaultsDoNotOverride() {
.addLine("DataType value = new DataType.Builder()")
.addLine(" .propertyA(11)")
.addLine(" .propertyB(true)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(3.2)")
.addLine(" .mergeFrom(new DataType.Builder())")
.addLine(" .build();")
.addLine("assertEquals(11, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"hello\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -125,10 +137,14 @@ public void testMergeFromValue_defaultsDoNotOverride() {
.addLine("DataType value = new DataType.Builder()")
.addLine(" .propertyA(11)")
.addLine(" .propertyB(true)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(3.2)")
.addLine(" .mergeFrom(new DataType.Builder().build())")
.addLine(" .build();")
.addLine("assertEquals(11, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"hello\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -143,9 +159,13 @@ public void testMergeFromBuilder_nonDefaultsUsed() {
.addLine(" .propertyB(true)")
.addLine(" .mergeFrom(new DataType.Builder())")
.addLine(" .propertyA(13)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(3.2)")
.addLine(" .build();")
.addLine("assertEquals(13, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"hello\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -160,10 +180,14 @@ public void testMergeFromValue_nonDefaultsUsed() {
.addLine(" .propertyB(true)")
.addLine(" .mergeFrom(new DataType.Builder()")
.addLine(" .propertyA(13)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(3.2)")
.addLine(" .build())")
.addLine(" .build();")
.addLine("assertEquals(13, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"hello\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -177,11 +201,17 @@ public void testMergeFromBuilder_nonDefaultsOverride() {
.addLine("DataType value = new DataType.Builder()")
.addLine(" .propertyA(11)")
.addLine(" .propertyB(true)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(1.9)")
.addLine(" .mergeFrom(new DataType.Builder())")
.addLine(" .propertyA(13)")
.addLine(" .propertyC(\"goodbye\")")
.addLine(" .propertyD(3.2)")
.addLine(" .build();")
.addLine("assertEquals(13, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"goodbye\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -195,12 +225,18 @@ public void testMergeFromValue_nonDefaultsOverride() {
.addLine("DataType value = new DataType.Builder()")
.addLine(" .propertyA(11)")
.addLine(" .propertyB(true)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(1.9)")
.addLine(" .mergeFrom(new DataType.Builder()")
.addLine(" .propertyA(13)")
.addLine(" .propertyC(\"goodbye\")")
.addLine(" .propertyD(3.2)")
.addLine(" .build())")
.addLine(" .build();")
.addLine("assertEquals(13, value.propertyA());")
.addLine("assertTrue(value.propertyB());")
.addLine("assertEquals(\"goodbye\", value.propertyC());")
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand All @@ -214,10 +250,14 @@ public void testClear() {
.addLine("DataType value = new DataType.Builder()")
.addLine(" .propertyA(11)")
.addLine(" .propertyB(true)")
.addLine(" .propertyC(\"hello\")")
.addLine(" .propertyD(1.9)")
.addLine(" .clear()")
.addLine(" .build();")
.addLine("assertEquals(0, value.propertyA());")
.addLine("assertFalse(value.propertyB());")
.addLine("assertEquals(\"default\", value.propertyC());")
.addLine("assertEquals(Double.NaN, value.propertyD(), 0.0001);")
.build())
.runTest();
}
Expand Down
Loading

0 comments on commit 1122b87

Please sign in to comment.