Review comments

Name Exceptions <blah>Exception
Swap null / non null annotations onto the correct classes
Don't shadow parameters with local vars
Explicitly handle Character Type
This commit is contained in:
Katelyn Baker 2017-07-06 14:09:14 +01:00
parent 32543021c9
commit 81b84ebf5c
2 changed files with 24 additions and 15 deletions

View File

@ -68,12 +68,15 @@ class ClassCarpenter {
// TODO: Support annotations.
// TODO: isFoo getter patterns for booleans (this is what Kotlin generates)
class DuplicateName : RuntimeException("An attempt was made to register two classes with the same name within the same ClassCarpenter namespace.")
class InterfaceMismatch(msg: String) : RuntimeException(msg)
class NullablePrimitive(msg: String) : RuntimeException(msg)
class DuplicateNameException : RuntimeException("An attempt was made to register two classes with the same name within the same ClassCarpenter namespace.")
class InterfaceMismatchException(msg: String) : RuntimeException(msg)
class NullablePrimitiveException(msg: String) : RuntimeException(msg)
abstract class Field(val field: Class<out Any?>) {
protected val unsetName = "Unset"
companion object {
const val unsetName = "Unset"
}
var name: String = unsetName
abstract val nullabilityAnnotation: String
@ -107,7 +110,7 @@ class ClassCarpenter {
}
class NonNullableField(field: Class<out Any?>) : Field(field) {
override val nullabilityAnnotation = "Ljavax/annotations/Nullable;"
override val nullabilityAnnotation = "Ljavax/annotations/NotNull;"
constructor(name: String, field: Class<out Any?>) : this(field) {
this.name = name
@ -135,11 +138,11 @@ class ClassCarpenter {
class NullableField(field: Class<out Any?>) : Field(field) {
override val nullabilityAnnotation = "Ljavax/annotations/NotNull;"
override val nullabilityAnnotation = "Ljavax/annotations/Nullable;"
constructor(name: String, field: Class<out Any?>) : this(field) {
if (field.isPrimitive) {
throw NullablePrimitive (
throw NullablePrimitiveException (
"Field $name is primitive type ${Type.getDescriptor(field)} and thus cannot be nullable")
}
@ -329,7 +332,8 @@ class ClassCarpenter {
visitVarInsn(ALOAD, 0) // Load 'this'
visitFieldInsn(GETFIELD, schema.jvmName, name, type.descriptor)
when (type.field) {
java.lang.Boolean.TYPE, Integer.TYPE, java.lang.Short.TYPE, java.lang.Byte.TYPE, TYPE -> visitInsn(IRETURN)
java.lang.Boolean.TYPE, Integer.TYPE, java.lang.Short.TYPE, java.lang.Byte.TYPE,
java.lang.Character.TYPE, TYPE -> visitInsn(IRETURN)
java.lang.Long.TYPE -> visitInsn(LRETURN)
java.lang.Double.TYPE -> visitInsn(DRETURN)
java.lang.Float.TYPE -> visitInsn(FRETURN)
@ -394,7 +398,8 @@ class ClassCarpenter {
private fun MethodVisitor.load(slot: Int, type: Field): Int {
when (type.field) {
java.lang.Boolean.TYPE, Integer.TYPE, java.lang.Short.TYPE, java.lang.Byte.TYPE, TYPE -> visitVarInsn(ILOAD, slot)
java.lang.Boolean.TYPE, Integer.TYPE, java.lang.Short.TYPE, java.lang.Byte.TYPE,
java.lang.Character.TYPE, TYPE -> visitVarInsn(ILOAD, slot)
java.lang.Long.TYPE -> visitVarInsn(LLOAD, slot)
java.lang.Double.TYPE -> visitVarInsn(DLOAD, slot)
java.lang.Float.TYPE -> visitVarInsn(FLOAD, slot)
@ -407,7 +412,7 @@ class ClassCarpenter {
}
private fun validateSchema(schema: Schema) {
if (schema.name in _loaded) throw DuplicateName()
if (schema.name in _loaded) throw DuplicateNameException()
fun isJavaName(n: String) = n.isNotBlank() && isJavaIdentifierStart(n.first()) && n.all(::isJavaIdentifierPart)
require(isJavaName(schema.name.split(".").last())) { "Not a valid Java name: ${schema.name}" }
schema.fields.keys.forEach { require(isJavaName(it)) { "Not a valid Java name: $it" } }
@ -419,11 +424,15 @@ class ClassCarpenter {
itf.methods.forEach {
val fieldNameFromItf = when {
it.name.startsWith("get") -> it.name.substring(3).decapitalize()
else -> throw InterfaceMismatch("Requested interfaces must consist only of methods that start with 'get': ${itf.name}.${it.name}")
else -> throw InterfaceMismatchException(
"Requested interfaces must consist only of methods that start "
+ "with 'get': ${itf.name}.${it.name}")
}
if ((schema is ClassSchema) and (fieldNameFromItf !in allFields))
throw InterfaceMismatch("Interface ${itf.name} requires a field named $fieldNameFromItf but that isn't found in the schema or any superclass schemas")
throw InterfaceMismatchException(
"Interface ${itf.name} requires a field named $fieldNameFromItf but that "
+ "isn't found in the schema or any superclass schemas")
}
}
}

View File

@ -90,7 +90,7 @@ class ClassCarpenterTest {
assertEquals("Person{age=32, name=Mike}", i.toString())
}
@Test(expected = ClassCarpenter.DuplicateName::class)
@Test(expected = ClassCarpenter.DuplicateNameException::class)
fun duplicates() {
cc.build(ClassCarpenter.ClassSchema("gen.EmptyClass", emptyMap(), null))
cc.build(ClassCarpenter.ClassSchema("gen.EmptyClass", emptyMap(), null))
@ -140,7 +140,7 @@ class ClassCarpenterTest {
assertEquals(1, i.b)
}
@Test(expected = ClassCarpenter.InterfaceMismatch::class)
@Test(expected = ClassCarpenter.InterfaceMismatchException::class)
fun `mismatched interface`() {
val schema1 = ClassCarpenter.ClassSchema(
"gen.A",
@ -268,7 +268,7 @@ class ClassCarpenterTest {
clazz.constructors[0].newInstance(a)
}
@Test(expected = ClassCarpenter.NullablePrimitive::class)
@Test(expected = ClassCarpenter.NullablePrimitiveException::class)
fun `nullable parameter small int`() {
val className = "iEnjoySwede"
val schema = ClassCarpenter.ClassSchema(