Skip to content
This repository was archived by the owner on Oct 16, 2024. It is now read-only.

Commit 1122b87

Browse files
alicederynalicederyn
authored
Merge pull request #242 from google/issue/240.merge.nans
Fix treatment of NaN defaults in mergeFrom
2 parents ac19c7e + 0393909 commit 1122b87

12 files changed

+459
-171
lines changed

‎src/main/java/org/inferred/freebuilder/processor/CodeGenerator.java

+9-20
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
import static org.inferred.freebuilder.processor.Metadata.GET_CODE_GENERATOR;
2323
import static org.inferred.freebuilder.processor.Metadata.UnderrideLevel.ABSENT;
2424
import static org.inferred.freebuilder.processor.Metadata.UnderrideLevel.FINAL;
25+
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NOT_NULLABLE;
26+
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NULLABLE;
2527
import static org.inferred.freebuilder.processor.util.feature.GuavaLibrary.GUAVA;
2628
import static org.inferred.freebuilder.processor.util.feature.SourceLevel.SOURCE_LEVEL;
2729

@@ -41,6 +43,7 @@
4143
import org.inferred.freebuilder.processor.util.Block;
4244
import org.inferred.freebuilder.processor.util.Excerpt;
4345
import org.inferred.freebuilder.processor.util.Excerpts;
46+
import org.inferred.freebuilder.processor.util.ObjectsExcerpts;
4447
import org.inferred.freebuilder.processor.util.PreconditionExcerpts;
4548
import org.inferred.freebuilder.processor.util.SourceBuilder;
4649

@@ -395,26 +398,12 @@ private static void addValueTypeEquals(SourceBuilder code, Metadata metadata) {
395398
code.add(";\n");
396399
} else {
397400
for (Property property : metadata.getProperties()) {
398-
switch (property.getType().getKind()) {
399-
case FLOAT:
400-
case DOUBLE:
401-
code.addLine(" if (%s.doubleToLongBits(%s)", Double.class, property.getName())
402-
.addLine(" != %s.doubleToLongBits(other.%s)) {",
403-
Double.class, property.getName());
404-
break;
405-
406-
default:
407-
if (property.getType().getKind().isPrimitive()) {
408-
code.addLine(" if (%1$s != other.%1$s) {", property.getName());
409-
} else if (property.getCodeGenerator().getType() == Type.OPTIONAL) {
410-
code.addLine(" if (%1$s != other.%1$s", property.getName())
411-
.addLine(" && (%1$s == null || !%1$s.equals(other.%1$s))) {",
412-
property.getName());
413-
} else {
414-
code.addLine(" if (!%1$s.equals(other.%1$s)) {", property.getName());
415-
}
416-
}
417-
code.addLine(" return false;")
401+
code.addLine(" if (%s) {", ObjectsExcerpts.notEquals(
402+
property.getName(),
403+
"other." + property.getName(),
404+
property.getType().getKind(),
405+
(property.getCodeGenerator().getType() == Type.OPTIONAL) ? NULLABLE : NOT_NULLABLE))
406+
.addLine(" return false;")
418407
.addLine(" }");
419408
}
420409
code.addLine(" return true;");

‎src/main/java/org/inferred/freebuilder/processor/DefaultPropertyFactory.java

+17-16
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static org.inferred.freebuilder.processor.BuilderMethods.getter;
2020
import static org.inferred.freebuilder.processor.BuilderMethods.mapper;
2121
import static org.inferred.freebuilder.processor.BuilderMethods.setter;
22+
import static org.inferred.freebuilder.processor.util.ObjectsExcerpts.Nullability.NOT_NULLABLE;
2223
import static org.inferred.freebuilder.processor.util.PreconditionExcerpts.checkNotNullInline;
2324
import static org.inferred.freebuilder.processor.util.PreconditionExcerpts.checkNotNullPreamble;
2425
import static org.inferred.freebuilder.processor.util.feature.FunctionPackage.FUNCTION_PACKAGE;
@@ -31,10 +32,12 @@
3132
import org.inferred.freebuilder.processor.util.Block;
3233
import org.inferred.freebuilder.processor.util.Excerpt;
3334
import org.inferred.freebuilder.processor.util.Excerpts;
35+
import org.inferred.freebuilder.processor.util.ObjectsExcerpts;
3436
import org.inferred.freebuilder.processor.util.ParameterizedType;
3537
import org.inferred.freebuilder.processor.util.PreconditionExcerpts;
3638
import org.inferred.freebuilder.processor.util.SourceBuilder;
3739

40+
import javax.lang.model.type.TypeKind;
3841
import javax.lang.model.type.TypeMirror;
3942

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

5255
private final boolean hasDefault;
53-
private final boolean isPrimitive;
56+
private final TypeKind kind;
5457

5558
CodeGenerator(Metadata metadata, Property property, boolean hasDefault) {
5659
super(metadata, property);
5760
this.hasDefault = hasDefault;
58-
this.isPrimitive = property.getType().getKind().isPrimitive();
61+
this.kind = property.getType().getKind();
5962
}
6063

6164
@Override
@@ -82,14 +85,14 @@ private void addSetter(SourceBuilder code, final Metadata metadata) {
8285
metadata.getType().javadocNoArgMethodLink(property.getGetterName()))
8386
.addLine(" *")
8487
.addLine(" * @return this {@code %s} object", metadata.getBuilder().getSimpleName());
85-
if (!isPrimitive) {
88+
if (!kind.isPrimitive()) {
8689
code.addLine(" * @throws NullPointerException if {@code %s} is null", property.getName());
8790
}
8891
code.addLine(" */");
8992
addAccessorAnnotations(code);
9093
code.addLine("public %s %s(%s %s) {",
9194
metadata.getBuilder(), setter(property), property.getType(), property.getName());
92-
if (isPrimitive) {
95+
if (kind.isPrimitive()) {
9396
code.addLine(" this.%1$s = %1$s;", property.getName());
9497
} else {
9598
code.add(checkNotNullPreamble(property.getName()))
@@ -176,13 +179,11 @@ public void addMergeFromValue(Block code, String value) {
176179
code.add("%s._unsetProperties.contains(%s.%s) || ",
177180
defaults, metadata.getPropertyEnum(), property.getAllCapsName());
178181
}
179-
if (isPrimitive) {
180-
code.add("%s.%s() != %s.%s()",
181-
value, property.getGetterName(), defaults, getter(property));
182-
} else {
183-
code.add("!%s.%s().equals(%s.%s())",
184-
value, property.getGetterName(), defaults, getter(property));
185-
}
182+
code.add(ObjectsExcerpts.notEquals(
183+
Excerpts.add("%s.%s()", value, property.getGetterName()),
184+
Excerpts.add("%s.%s()", defaults, getter(property)),
185+
kind,
186+
NOT_NULLABLE));
186187
code.add(") {%n");
187188
}
188189
code.addLine(" %s(%s.%s());", setter(property), value, property.getGetterName());
@@ -204,11 +205,11 @@ public void addMergeFromBuilder(Block code, String builder) {
204205
.add("(%s._unsetProperties.contains(%s.%s) ||",
205206
defaults, metadata.getPropertyEnum(), property.getAllCapsName());
206207
}
207-
if (isPrimitive) {
208-
code.add("%1$s.%2$s() != %3$s.%2$s()", builder, getter(property), defaults);
209-
} else {
210-
code.add("!%1$s.%2$s().equals(%3$s.%2$s())", builder, getter(property), defaults);
211-
}
208+
code.add(ObjectsExcerpts.notEquals(
209+
Excerpts.add("%s.%s()", builder, getter(property)),
210+
Excerpts.add("%s.%s()", defaults, getter(property)),
211+
kind,
212+
NOT_NULLABLE));
212213
if (!hasDefault) {
213214
code.add(")");
214215
}

‎src/main/java/org/inferred/freebuilder/processor/util/ObjectsExcerpts.java

+98
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package org.inferred.freebuilder.processor.util;
2+
3+
import static org.inferred.freebuilder.processor.util.feature.SourceLevel.SOURCE_LEVEL;
4+
5+
import javax.lang.model.type.TypeKind;
6+
7+
public class ObjectsExcerpts {
8+
9+
public enum Nullability {
10+
NULLABLE, NOT_NULLABLE;
11+
12+
public boolean isNullable() {
13+
return this == NULLABLE;
14+
}
15+
}
16+
17+
/**
18+
* Returns an Excerpt equivalent to {@code Objects.equals(a, b)}.
19+
*
20+
* <p>If Objects is not available, {@code kind} and {@code nullability} are needed to generate
21+
* the most idiomatic equivalent.
22+
*/
23+
public static Excerpt equals(Object a, Object b, TypeKind kind, Nullability nullability) {
24+
return new EqualsExcerpt(true, a, b, kind, nullability);
25+
}
26+
27+
/**
28+
* Returns an Excerpt equivalent to {@code !Objects.equals(a, b)}.
29+
*
30+
* <p>If Objects is not available, {@code kind} and {@code nullability} are needed to generate
31+
* the most idiomatic equivalent.
32+
*/
33+
public static Excerpt notEquals(Object a, Object b, TypeKind kind, Nullability nullability) {
34+
return new EqualsExcerpt(false, a, b, kind, nullability);
35+
}
36+
37+
private static class EqualsExcerpt extends Excerpt {
38+
39+
private final boolean areEqual;
40+
private final Object a;
41+
private final Object b;
42+
private final TypeKind kind;
43+
private final Nullability nullability;
44+
45+
EqualsExcerpt(boolean areEqual, Object a, Object b, TypeKind kind, Nullability nullability) {
46+
this.areEqual = areEqual;
47+
this.a = a;
48+
this.b = b;
49+
this.kind = kind;
50+
this.nullability = nullability;
51+
}
52+
53+
@Override
54+
public void addTo(SourceBuilder code) {
55+
QualifiedName javaUtilObjects = code.feature(SOURCE_LEVEL).javaUtilObjects().orNull();
56+
if (javaUtilObjects != null) {
57+
code.add("%s%s.equals(%s, %s)", areEqual ? "" : "!", javaUtilObjects, a, b);
58+
} else if (nullability.isNullable()) {
59+
if (areEqual) {
60+
code.add("%1$s == %2$s || (%1$s != null && %1$s.equals(%2$s))", a, b);
61+
} else {
62+
code.add("%1$s != %2$s && (%1$s == null || !%1$s.equals(%2$s))", a, b);
63+
}
64+
} else if (kind.isPrimitive()) {
65+
switch (kind) {
66+
case FLOAT:
67+
code.add("%1$s.floatToIntBits(%2$s) %3$s %1$s.floatToIntBits(%4$s)",
68+
Float.class, a, areEqual ? "==" : "!=", b);
69+
break;
70+
71+
case DOUBLE:
72+
code.add("%1$s.doubleToLongBits(%2$s) %3$s %1$s.doubleToLongBits(%4$s)",
73+
Double.class, a, areEqual ? "==" : "!=", b);
74+
break;
75+
76+
default:
77+
code.add("%s %s %s", a, areEqual ? "==" : "!=", b);
78+
}
79+
} else {
80+
code.add("%s%s.equals(%s)", areEqual ? "" : "!", a, b);
81+
}
82+
}
83+
84+
@Override
85+
protected void addFields(FieldReceiver fields) {
86+
fields.add("areEqual", areEqual);
87+
fields.add("a", a);
88+
fields.add("b", b);
89+
fields.add("kind", kind);
90+
fields.add("nullable", nullability);
91+
}
92+
93+
}
94+
95+
96+
97+
private ObjectsExcerpts() {}
98+
}

‎src/main/java/org/inferred/freebuilder/processor/util/SourceStringBuilder.java

+8
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@ public static SourceBuilder simple(Feature<?>... features) {
5050
new TypeShortener.AlwaysShorten(), new StaticFeatureSet(features));
5151
}
5252

53+
/**
54+
* Returns a {@link SourceStringBuilder} that returns compilable code.
55+
*/
56+
public static SourceBuilder compilable(FeatureSet features) {
57+
return new SourceStringBuilder(
58+
new TypeShortener.NeverShorten(), features);
59+
}
60+
5361
SourceStringBuilder(TypeShortener shortener, FeatureSet features) {
5462
this.shortener = shortener;
5563
this.features = features;

‎src/test/java/org/inferred/freebuilder/processor/DefaultedPrefixlessPropertiesTest.java

+40
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,15 @@ public static Iterable<Object[]> featureSets() {
6262
.addLine("public interface DataType {")
6363
.addLine(" int propertyA();")
6464
.addLine(" boolean propertyB();")
65+
.addLine(" String propertyC();")
66+
.addLine(" double propertyD();")
6567
.addLine("")
6668
.addLine(" class Builder extends DataType_Builder {")
6769
.addLine(" public Builder() {")
6870
.addLine(" propertyA(0);")
6971
.addLine(" propertyB(false);")
72+
.addLine(" propertyC(\"default\");")
73+
.addLine(" propertyD(Double.NaN);")
7074
.addLine(" }")
7175
.addLine(" }")
7276
.addLine("}")
@@ -82,12 +86,16 @@ public static Iterable<Object[]> featureSets() {
8286
.addLine("public interface DataType {")
8387
.addLine(" int propertyA();")
8488
.addLine(" boolean propertyB();")
89+
.addLine(" String propertyC();")
90+
.addLine(" double propertyD();")
8591
.addLine("")
8692
.addLine(" class Builder extends DataType_Builder {")
8793
.addLine(" public Builder() {")
8894
.addLine(" if (true) { // Disable optimization in javac")
8995
.addLine(" propertyA(0);")
9096
.addLine(" propertyB(false);")
97+
.addLine(" propertyC(\"default\");")
98+
.addLine(" propertyD(Double.NaN);")
9199
.addLine(" }")
92100
.addLine(" }")
93101
.addLine(" }")
@@ -108,10 +116,14 @@ public void testMergeFromBuilder_defaultsDoNotOverride() {
108116
.addLine("DataType value = new DataType.Builder()")
109117
.addLine(" .propertyA(11)")
110118
.addLine(" .propertyB(true)")
119+
.addLine(" .propertyC(\"hello\")")
120+
.addLine(" .propertyD(3.2)")
111121
.addLine(" .mergeFrom(new DataType.Builder())")
112122
.addLine(" .build();")
113123
.addLine("assertEquals(11, value.propertyA());")
114124
.addLine("assertTrue(value.propertyB());")
125+
.addLine("assertEquals(\"hello\", value.propertyC());")
126+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
115127
.build())
116128
.runTest();
117129
}
@@ -125,10 +137,14 @@ public void testMergeFromValue_defaultsDoNotOverride() {
125137
.addLine("DataType value = new DataType.Builder()")
126138
.addLine(" .propertyA(11)")
127139
.addLine(" .propertyB(true)")
140+
.addLine(" .propertyC(\"hello\")")
141+
.addLine(" .propertyD(3.2)")
128142
.addLine(" .mergeFrom(new DataType.Builder().build())")
129143
.addLine(" .build();")
130144
.addLine("assertEquals(11, value.propertyA());")
131145
.addLine("assertTrue(value.propertyB());")
146+
.addLine("assertEquals(\"hello\", value.propertyC());")
147+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
132148
.build())
133149
.runTest();
134150
}
@@ -143,9 +159,13 @@ public void testMergeFromBuilder_nonDefaultsUsed() {
143159
.addLine(" .propertyB(true)")
144160
.addLine(" .mergeFrom(new DataType.Builder())")
145161
.addLine(" .propertyA(13)")
162+
.addLine(" .propertyC(\"hello\")")
163+
.addLine(" .propertyD(3.2)")
146164
.addLine(" .build();")
147165
.addLine("assertEquals(13, value.propertyA());")
148166
.addLine("assertTrue(value.propertyB());")
167+
.addLine("assertEquals(\"hello\", value.propertyC());")
168+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
149169
.build())
150170
.runTest();
151171
}
@@ -160,10 +180,14 @@ public void testMergeFromValue_nonDefaultsUsed() {
160180
.addLine(" .propertyB(true)")
161181
.addLine(" .mergeFrom(new DataType.Builder()")
162182
.addLine(" .propertyA(13)")
183+
.addLine(" .propertyC(\"hello\")")
184+
.addLine(" .propertyD(3.2)")
163185
.addLine(" .build())")
164186
.addLine(" .build();")
165187
.addLine("assertEquals(13, value.propertyA());")
166188
.addLine("assertTrue(value.propertyB());")
189+
.addLine("assertEquals(\"hello\", value.propertyC());")
190+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
167191
.build())
168192
.runTest();
169193
}
@@ -177,11 +201,17 @@ public void testMergeFromBuilder_nonDefaultsOverride() {
177201
.addLine("DataType value = new DataType.Builder()")
178202
.addLine(" .propertyA(11)")
179203
.addLine(" .propertyB(true)")
204+
.addLine(" .propertyC(\"hello\")")
205+
.addLine(" .propertyD(1.9)")
180206
.addLine(" .mergeFrom(new DataType.Builder())")
181207
.addLine(" .propertyA(13)")
208+
.addLine(" .propertyC(\"goodbye\")")
209+
.addLine(" .propertyD(3.2)")
182210
.addLine(" .build();")
183211
.addLine("assertEquals(13, value.propertyA());")
184212
.addLine("assertTrue(value.propertyB());")
213+
.addLine("assertEquals(\"goodbye\", value.propertyC());")
214+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
185215
.build())
186216
.runTest();
187217
}
@@ -195,12 +225,18 @@ public void testMergeFromValue_nonDefaultsOverride() {
195225
.addLine("DataType value = new DataType.Builder()")
196226
.addLine(" .propertyA(11)")
197227
.addLine(" .propertyB(true)")
228+
.addLine(" .propertyC(\"hello\")")
229+
.addLine(" .propertyD(1.9)")
198230
.addLine(" .mergeFrom(new DataType.Builder()")
199231
.addLine(" .propertyA(13)")
232+
.addLine(" .propertyC(\"goodbye\")")
233+
.addLine(" .propertyD(3.2)")
200234
.addLine(" .build())")
201235
.addLine(" .build();")
202236
.addLine("assertEquals(13, value.propertyA());")
203237
.addLine("assertTrue(value.propertyB());")
238+
.addLine("assertEquals(\"goodbye\", value.propertyC());")
239+
.addLine("assertEquals(3.2, value.propertyD(), 0.0001);")
204240
.build())
205241
.runTest();
206242
}
@@ -214,10 +250,14 @@ public void testClear() {
214250
.addLine("DataType value = new DataType.Builder()")
215251
.addLine(" .propertyA(11)")
216252
.addLine(" .propertyB(true)")
253+
.addLine(" .propertyC(\"hello\")")
254+
.addLine(" .propertyD(1.9)")
217255
.addLine(" .clear()")
218256
.addLine(" .build();")
219257
.addLine("assertEquals(0, value.propertyA());")
220258
.addLine("assertFalse(value.propertyB());")
259+
.addLine("assertEquals(\"default\", value.propertyC());")
260+
.addLine("assertEquals(Double.NaN, value.propertyD(), 0.0001);")
221261
.build())
222262
.runTest();
223263
}

0 commit comments

Comments
 (0)