From c997c136bbd5a4f7131d21a3f8c45e563fa5cca5 Mon Sep 17 00:00:00 2001 From: Andrew Flynn Date: Wed, 4 Dec 2013 13:10:07 -0800 Subject: [PATCH] Nano: don't generate accessor methods for nested methods For nested message objects, don't generate accessor methods because they have a default value that is not a valid value (null), so there is no reason to have get/set/has/clear methods for them. Clients and protos (while serializing) can check against the invalid value to see if it's been set. Change-Id: Ic63400889581271b8cbcd9c45c84519d4921fd4b --- java/README.txt | 6 +- .../java/com/google/protobuf/NanoTest.java | 60 ++++++++---- .../compiler/javanano/javanano_field.cc | 12 +-- .../javanano/javanano_message_field.cc | 91 ------------------- .../javanano/javanano_message_field.h | 22 ----- 5 files changed, 53 insertions(+), 138 deletions(-) diff --git a/java/README.txt b/java/README.txt index 976ec842a3..ced78d231f 100644 --- a/java/README.txt +++ b/java/README.txt @@ -507,9 +507,9 @@ optional_field_style={default,accessors,reftypes} (default: default) useful when you need to serialize a field with the default value, or check if a field has been explicitly set to its default value from the wire. - In the 'accessors' style, required fields are still translated to one - public mutable Java field each, and repeated fields are still translated - to arrays. No accessors are generated for them. + In the 'accessors' style, required and nested message fields are still + translated to one public mutable Java field each, repeated fields are still + translated to arrays. No accessors are generated for them. IMPORTANT: When using the 'accessors' style, ProGuard should always be enabled with optimization (don't use -dontoptimize) and allowing diff --git a/java/src/test/java/com/google/protobuf/NanoTest.java b/java/src/test/java/com/google/protobuf/NanoTest.java index 0e1c9bc93b..4b8d63d91c 100644 --- a/java/src/test/java/com/google/protobuf/NanoTest.java +++ b/java/src/test/java/com/google/protobuf/NanoTest.java @@ -2271,9 +2271,10 @@ public class NanoTest extends TestCase { public void testNanoWithAccessorsBasic() throws Exception { TestNanoAccessors msg = new TestNanoAccessors(); - // Makes sure required and repeated fields are still public fields + // Makes sure required, repeated, and message fields are still public msg.id = 3; msg.repeatedBytes = new byte[2][3]; + msg.optionalNestedMessage = null; // Test accessors assertEquals(0, msg.getOptionalInt32()); @@ -2295,10 +2296,6 @@ public class NanoTest extends TestCase { msg.setOptionalString(null); fail(); } catch (NullPointerException expected) {} - try { - msg.setOptionalNestedMessage(null); - fail(); - } catch (NullPointerException expected) {} // Test has bit on bytes field with defaults and clear() re-clones the default array assertFalse(msg.hasDefaultBytes()); @@ -2352,8 +2349,8 @@ public class NanoTest extends TestCase { assertFalse(msg.hasDefaultBytes()); assertFalse(msg.hasDefaultFloatNan()); assertFalse(msg.hasDefaultNestedEnum()); - msg.setOptionalNestedMessage(new TestNanoAccessors.NestedMessage()); - msg.getOptionalNestedMessage().setBb(2); + msg.optionalNestedMessage = new TestNanoAccessors.NestedMessage(); + msg.optionalNestedMessage.setBb(2); msg.setOptionalNestedEnum(TestNanoAccessors.BAZ); msg.setDefaultInt32(msg.getDefaultInt32()); } @@ -2366,8 +2363,8 @@ public class NanoTest extends TestCase { // Has fields true upon parse. TestNanoAccessors newMsg = TestNanoAccessors.parseFrom(result); - assertEquals(2, newMsg.getOptionalNestedMessage().getBb()); - assertTrue(newMsg.getOptionalNestedMessage().hasBb()); + assertEquals(2, newMsg.optionalNestedMessage.getBb()); + assertTrue(newMsg.optionalNestedMessage.hasBb()); assertEquals(TestNanoAccessors.BAZ, newMsg.getOptionalNestedEnum()); assertTrue(newMsg.hasOptionalNestedEnum()); @@ -2376,6 +2373,38 @@ public class NanoTest extends TestCase { assertEquals(41, newMsg.getDefaultInt32()); } + public void testNanoWithAccessorsPublicFieldTypes() throws Exception { + TestNanoAccessors msg = new TestNanoAccessors(); + assertNull(msg.optionalNestedMessage); + assertEquals(0, msg.id); + assertEquals(0, msg.repeatedNestedEnum.length); + + TestNanoAccessors newMsg = TestNanoAccessors.parseFrom(MessageNano.toByteArray(msg)); + assertNull(newMsg.optionalNestedMessage); + assertEquals(0, newMsg.id); + assertEquals(0, newMsg.repeatedNestedEnum.length); + + TestNanoAccessors.NestedMessage nestedMessage = new TestNanoAccessors.NestedMessage(); + nestedMessage.setBb(5); + newMsg.optionalNestedMessage = nestedMessage; + newMsg.id = -1; + newMsg.repeatedNestedEnum = new int[] { TestAllTypesNano.FOO }; + + TestNanoAccessors newMsg2 = TestNanoAccessors.parseFrom(MessageNano.toByteArray(newMsg)); + assertEquals(nestedMessage.getBb(), newMsg2.optionalNestedMessage.getBb()); + assertEquals(-1, newMsg2.id); + assertEquals(TestAllTypesNano.FOO, newMsg2.repeatedNestedEnum[0]); + + newMsg2.optionalNestedMessage = null; + newMsg2.id = 0; + newMsg2.repeatedNestedEnum = null; + + TestNanoAccessors newMsg3 = TestNanoAccessors.parseFrom(MessageNano.toByteArray(newMsg2)); + assertNull(newMsg3.optionalNestedMessage); + assertEquals(0, newMsg3.id); + assertEquals(0, newMsg3.repeatedNestedEnum.length); + } + public void testNanoWithAccessorsSerialize() throws Exception { TestNanoAccessors msg = new TestNanoAccessors(); msg.setOptionalInt32(msg.getOptionalInt32()); @@ -2383,7 +2412,7 @@ public class NanoTest extends TestCase { msg.setOptionalBytes(msg.getOptionalBytes()); TestNanoAccessors.NestedMessage nestedMessage = new TestNanoAccessors.NestedMessage(); nestedMessage.setBb(nestedMessage.getBb()); - msg.setOptionalNestedMessage(nestedMessage); + msg.optionalNestedMessage = nestedMessage; msg.setOptionalNestedEnum(msg.getOptionalNestedEnum()); msg.setDefaultInt32(msg.getDefaultInt32()); msg.setDefaultString(msg.getDefaultString()); @@ -2400,8 +2429,7 @@ public class NanoTest extends TestCase { assertTrue(newMsg.hasOptionalInt32()); assertTrue(newMsg.hasOptionalString()); assertTrue(newMsg.hasOptionalBytes()); - assertTrue(newMsg.hasOptionalNestedMessage()); - assertTrue(newMsg.getOptionalNestedMessage().hasBb()); + assertTrue(newMsg.optionalNestedMessage.hasBb()); assertTrue(newMsg.hasOptionalNestedEnum()); assertTrue(newMsg.hasDefaultInt32()); assertTrue(newMsg.hasDefaultString()); @@ -2411,7 +2439,7 @@ public class NanoTest extends TestCase { assertEquals(0, newMsg.getOptionalInt32()); assertEquals(0, newMsg.getOptionalString().length()); assertEquals(0, newMsg.getOptionalBytes().length); - assertEquals(0, newMsg.getOptionalNestedMessage().getBb()); + assertEquals(0, newMsg.optionalNestedMessage.getBb()); assertEquals(TestNanoAccessors.FOO, newMsg.getOptionalNestedEnum()); assertEquals(41, newMsg.getDefaultInt32()); assertEquals("hello", newMsg.getDefaultString()); @@ -2856,15 +2884,15 @@ public class NanoTest extends TestCase { .setOptionalInt32(5) .setOptionalString("Hello") .setOptionalBytes(new byte[] {1, 2, 3}) - .setOptionalNestedMessage(new TestNanoAccessors.NestedMessage().setBb(27)) .setOptionalNestedEnum(TestNanoAccessors.BAR) .setDefaultFloatNan(1.0f); + message.optionalNestedMessage = new TestNanoAccessors.NestedMessage().setBb(27); message.repeatedInt32 = new int[] { 5, 6, 7, 8 }; message.repeatedString = new String[] { "One", "Two" }; message.repeatedBytes = new byte[][] { { 2, 7 }, { 2, 7 } }; message.repeatedNestedMessage = new TestNanoAccessors.NestedMessage[] { - message.getOptionalNestedMessage(), - message.getOptionalNestedMessage() + message.optionalNestedMessage, + message.optionalNestedMessage }; message.repeatedNestedEnum = new int[] { TestAllTypesNano.BAR, diff --git a/src/google/protobuf/compiler/javanano/javanano_field.cc b/src/google/protobuf/compiler/javanano/javanano_field.cc index 62e133e7f5..2581669909 100644 --- a/src/google/protobuf/compiler/javanano/javanano_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_field.cc @@ -78,8 +78,9 @@ FieldGeneratorMap::FieldGeneratorMap(const Descriptor* descriptor, const Params FieldGenerator* FieldGeneratorMap::MakeGenerator(const FieldDescriptor* field, const Params ¶ms, int* next_has_bit_index) { + JavaType java_type = GetJavaType(field); if (field->is_repeated()) { - switch (GetJavaType(field)) { + switch (java_type) { case JAVATYPE_MESSAGE: return new RepeatedMessageFieldGenerator(field, params); case JAVATYPE_ENUM: @@ -87,14 +88,13 @@ FieldGenerator* FieldGeneratorMap::MakeGenerator(const FieldDescriptor* field, default: return new RepeatedPrimitiveFieldGenerator(field, params); } - } else if (params.optional_field_accessors() && field->is_optional()) { + } else if (params.optional_field_accessors() && field->is_optional() + && java_type != JAVATYPE_MESSAGE) { // We need a has-bit for each primitive/enum field because their default // values could be same as explicitly set values. But we don't need it // for a message field because they have no defaults and Nano uses 'null' // for unset messages, which cannot be set explicitly. - switch (GetJavaType(field)) { - case JAVATYPE_MESSAGE: - return new AccessorMessageFieldGenerator(field, params); + switch (java_type) { case JAVATYPE_ENUM: return new AccessorEnumFieldGenerator( field, params, (*next_has_bit_index)++); @@ -103,7 +103,7 @@ FieldGenerator* FieldGeneratorMap::MakeGenerator(const FieldDescriptor* field, field, params, (*next_has_bit_index)++); } } else { - switch (GetJavaType(field)) { + switch (java_type) { case JAVATYPE_MESSAGE: return new MessageFieldGenerator(field, params); case JAVATYPE_ENUM: diff --git a/src/google/protobuf/compiler/javanano/javanano_message_field.cc b/src/google/protobuf/compiler/javanano/javanano_message_field.cc index d9abea36e6..74d3f85f42 100644 --- a/src/google/protobuf/compiler/javanano/javanano_message_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_message_field.cc @@ -149,97 +149,6 @@ GenerateHashCodeCode(io::Printer* printer) const { // =================================================================== -AccessorMessageFieldGenerator:: -AccessorMessageFieldGenerator( - const FieldDescriptor* descriptor, const Params& params) - : FieldGenerator(params), descriptor_(descriptor) { - SetMessageVariables(params, descriptor, &variables_); -} - -AccessorMessageFieldGenerator::~AccessorMessageFieldGenerator() {} - -void AccessorMessageFieldGenerator:: -GenerateMembers(io::Printer* printer) const { - printer->Print(variables_, - "private $type$ $name$_;\n" - "public $type$ get$capitalized_name$() {\n" - " return $name$_;\n" - "}\n" - "public $message_name$ set$capitalized_name$($type$ value) {\n" - " if (value == null) {\n" - " throw new java.lang.NullPointerException();\n" - " }\n" - " $name$_ = value;\n" - " return this;\n" - "}\n" - "public boolean has$capitalized_name$() {\n" - " return $name$_ != null;\n" - "}\n" - "public $message_name$ clear$capitalized_name$() {\n" - " $name$_ = null;\n" - " return this;" - "}\n"); -} - -void AccessorMessageFieldGenerator:: -GenerateClearCode(io::Printer* printer) const { - printer->Print(variables_, - "$name$_ = null;\n"); -} - -void AccessorMessageFieldGenerator:: -GenerateMergingCode(io::Printer* printer) const { - printer->Print(variables_, - "if ($name$_ == null) {\n" - " $name$_ = new $type$();\n" - "}\n"); - - if (descriptor_->type() == FieldDescriptor::TYPE_GROUP) { - printer->Print(variables_, - "input.readGroup($name$_, $number$);\n"); - } else { - printer->Print(variables_, - "input.readMessage($name$_);\n"); - } -} - -void AccessorMessageFieldGenerator:: -GenerateSerializationCode(io::Printer* printer) const { - printer->Print(variables_, - "if ($name$_ != null) {\n" - " output.write$group_or_message$($number$, $name$_);\n" - "}\n"); -} - -void AccessorMessageFieldGenerator:: -GenerateSerializedSizeCode(io::Printer* printer) const { - printer->Print(variables_, - "if ($name$_ != null) {\n" - " size += com.google.protobuf.nano.CodedOutputByteBufferNano\n" - " .compute$group_or_message$Size($number$, $name$_);\n" - "}\n"); -} - -void AccessorMessageFieldGenerator:: -GenerateEqualsCode(io::Printer* printer) const { - printer->Print(variables_, - "if ($name$_ == null) {\n" - " if (other.$name$_ != null) {\n" - " return false;\n" - " }\n" - "} else if (!$name$_.equals(other.$name$_)) {\n" - " return false;\n" - "}\n"); -} - -void AccessorMessageFieldGenerator:: -GenerateHashCodeCode(io::Printer* printer) const { - printer->Print(variables_, - "result = 31 * result + ($name$_ == null ? 0 : $name$_.hashCode());\n"); -} - -// =================================================================== - RepeatedMessageFieldGenerator:: RepeatedMessageFieldGenerator(const FieldDescriptor* descriptor, const Params& params) : FieldGenerator(params), descriptor_(descriptor) { diff --git a/src/google/protobuf/compiler/javanano/javanano_message_field.h b/src/google/protobuf/compiler/javanano/javanano_message_field.h index b724351dc9..55e2610cae 100644 --- a/src/google/protobuf/compiler/javanano/javanano_message_field.h +++ b/src/google/protobuf/compiler/javanano/javanano_message_field.h @@ -65,28 +65,6 @@ class MessageFieldGenerator : public FieldGenerator { GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(MessageFieldGenerator); }; -class AccessorMessageFieldGenerator : public FieldGenerator { - public: - explicit AccessorMessageFieldGenerator( - const FieldDescriptor* descriptor, const Params& params); - ~AccessorMessageFieldGenerator(); - - // implements FieldGenerator --------------------------------------- - void GenerateMembers(io::Printer* printer) const; - void GenerateClearCode(io::Printer* printer) const; - void GenerateMergingCode(io::Printer* printer) const; - void GenerateSerializationCode(io::Printer* printer) const; - void GenerateSerializedSizeCode(io::Printer* printer) const; - void GenerateEqualsCode(io::Printer* printer) const; - void GenerateHashCodeCode(io::Printer* printer) const; - - private: - const FieldDescriptor* descriptor_; - map variables_; - - GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(AccessorMessageFieldGenerator); -}; - class RepeatedMessageFieldGenerator : public FieldGenerator { public: explicit RepeatedMessageFieldGenerator(const FieldDescriptor* descriptor,