From 382ddccb550e1c822ef26a0e65988998f7446624 Mon Sep 17 00:00:00 2001 From: Max Cai Date: Wed, 20 Nov 2013 18:59:01 +0000 Subject: Extension overhaul. - Get rid of TypeLiteral. It was introduced to read the component type of a List at runtime. But we use arrays everywhere else, and we can always read the component type of an array type at runtime. - Properly read/write "minor" types (e.g. sint32, sfixed32). The old implementation could only read/write data as the "typical" types (one per Java type), e.g. java.lang.Integer -> int32, java.lang.Long -> int64. So if e.g. an extension specifies sfixed32 as the type, it would be read/written in the totally incompatible int32 format. - Properly serialize repeated packed fields. The old implementation doesn't do packed serialization. As an added bonus, and to be more aligned with the rest of protobuf nano / main, repeated packable extensions can deserialize both packed and non-packed data. - Split Extension class into a hierarchy so under typical usage a large chunk of code dealing with primitive type extensions can be removed by ProGuard. Bug: https://code.google.com/p/android/issues/detail?id=62586 Change-Id: I0d692f35cc2a8ad3a5a1cb3ce001282b2356b041 --- .../compiler/javanano/javanano_extension.cc | 113 +++++++++++++++------ .../protobuf/compiler/javanano/javanano_helpers.cc | 38 ++++++- .../protobuf/compiler/javanano/javanano_helpers.h | 8 +- .../protobuf/compiler/javanano/javanano_message.cc | 92 ++++++----------- .../compiler/javanano/javanano_primitive_field.cc | 20 ---- src/google/protobuf/unittest_extension_nano.proto | 21 +--- .../protobuf/unittest_extension_packed_nano.proto | 29 ++++++ .../unittest_extension_repeated_nano.proto | 34 +++++++ .../unittest_extension_singular_nano.proto | 34 +++++++ 9 files changed, 258 insertions(+), 131 deletions(-) create mode 100644 src/google/protobuf/unittest_extension_packed_nano.proto create mode 100644 src/google/protobuf/unittest_extension_repeated_nano.proto create mode 100644 src/google/protobuf/unittest_extension_singular_nano.proto (limited to 'src') diff --git a/src/google/protobuf/compiler/javanano/javanano_extension.cc b/src/google/protobuf/compiler/javanano/javanano_extension.cc index 0bc9c9d..754ed55 100644 --- a/src/google/protobuf/compiler/javanano/javanano_extension.cc +++ b/src/google/protobuf/compiler/javanano/javanano_extension.cc @@ -42,28 +42,84 @@ namespace compiler { namespace javanano { using internal::WireFormat; +using internal::WireFormatLite; + +namespace { + +const char* GetTypeConstantName(const FieldDescriptor::Type type) { + switch (type) { + case FieldDescriptor::TYPE_INT32 : return "TYPE_INT32" ; + case FieldDescriptor::TYPE_UINT32 : return "TYPE_UINT32" ; + case FieldDescriptor::TYPE_SINT32 : return "TYPE_SINT32" ; + case FieldDescriptor::TYPE_FIXED32 : return "TYPE_FIXED32" ; + case FieldDescriptor::TYPE_SFIXED32: return "TYPE_SFIXED32"; + case FieldDescriptor::TYPE_INT64 : return "TYPE_INT64" ; + case FieldDescriptor::TYPE_UINT64 : return "TYPE_UINT64" ; + case FieldDescriptor::TYPE_SINT64 : return "TYPE_SINT64" ; + case FieldDescriptor::TYPE_FIXED64 : return "TYPE_FIXED64" ; + case FieldDescriptor::TYPE_SFIXED64: return "TYPE_SFIXED64"; + case FieldDescriptor::TYPE_FLOAT : return "TYPE_FLOAT" ; + case FieldDescriptor::TYPE_DOUBLE : return "TYPE_DOUBLE" ; + case FieldDescriptor::TYPE_BOOL : return "TYPE_BOOL" ; + case FieldDescriptor::TYPE_STRING : return "TYPE_STRING" ; + case FieldDescriptor::TYPE_BYTES : return "TYPE_BYTES" ; + case FieldDescriptor::TYPE_ENUM : return "TYPE_ENUM" ; + case FieldDescriptor::TYPE_GROUP : return "TYPE_GROUP" ; + case FieldDescriptor::TYPE_MESSAGE : return "TYPE_MESSAGE" ; + + // No default because we want the compiler to complain if any new + // types are added. + } + + GOOGLE_LOG(FATAL) << "Can't get here."; + return NULL; +} + +} // namespace void SetVariables(const FieldDescriptor* descriptor, const Params params, map* variables) { - (*variables)["name"] = - RenameJavaKeywords(UnderscoresToCamelCase(descriptor)); - (*variables)["number"] = SimpleItoa(descriptor->number()); (*variables)["extends"] = ClassName(params, descriptor->containing_type()); - - string type; + (*variables)["name"] = RenameJavaKeywords(UnderscoresToCamelCase(descriptor)); + bool repeated = descriptor->is_repeated(); + (*variables)["repeated"] = repeated ? "Repeated" : ""; + (*variables)["type"] = GetTypeConstantName(descriptor->type()); JavaType java_type = GetJavaType(descriptor->type()); - switch (java_type) { - case JAVATYPE_ENUM: - type = "java.lang.Integer"; - break; - case JAVATYPE_MESSAGE: - type = ClassName(params, descriptor->message_type()); - break; - default: - type = BoxedPrimitiveTypeName(java_type); - break; + string tag = SimpleItoa(WireFormat::MakeTag(descriptor)); + if (java_type == JAVATYPE_MESSAGE) { + (*variables)["ext_type"] = "MessageTyped"; + string message_type = ClassName(params, descriptor->message_type()); + if (repeated) { + message_type += "[]"; + } + (*variables)["class"] = message_type; + // For message typed extensions, tags_params contains a single tag + // for both singular and repeated cases. + (*variables)["tag_params"] = tag; + } else { + (*variables)["ext_type"] = "PrimitiveTyped"; + if (!repeated) { + (*variables)["class"] = BoxedPrimitiveTypeName(java_type); + (*variables)["tag_params"] = tag; + } else { + (*variables)["class"] = PrimitiveTypeName(java_type) + "[]"; + if (!descriptor->is_packable()) { + // Non-packable: nonPackedTag == tag, packedTag == 0 + (*variables)["tag_params"] = tag + ", " + tag + ", 0"; + } else if (descriptor->options().packed()) { + // Packable and packed: tag == packedTag + string non_packed_tag = SimpleItoa(WireFormatLite::MakeTag( + descriptor->number(), + WireFormat::WireTypeForFieldType(descriptor->type()))); + (*variables)["tag_params"] = tag + ", " + non_packed_tag + ", " + tag; + } else { + // Packable and not packed: tag == nonPackedTag + string packed_tag = SimpleItoa(WireFormatLite::MakeTag( + descriptor->number(), WireFormatLite::WIRETYPE_LENGTH_DELIMITED)); + (*variables)["tag_params"] = tag + ", " + tag + ", " + packed_tag; + } + } } - (*variables)["type"] = type; } ExtensionGenerator:: @@ -75,21 +131,16 @@ ExtensionGenerator(const FieldDescriptor* descriptor, const Params& params) ExtensionGenerator::~ExtensionGenerator() {} void ExtensionGenerator::Generate(io::Printer* printer) const { - if (descriptor_->is_repeated()) { - printer->Print(variables_, - "\n" - "// extends $extends$\n" - "public static final com.google.protobuf.nano.Extension> $name$ = \n" - " com.google.protobuf.nano.Extension.createRepeated($number$,\n" - " new com.google.protobuf.nano.Extension.TypeLiteral>(){});\n"); - } else { - printer->Print(variables_, - "\n" - "// extends $extends$\n" - "public static final com.google.protobuf.nano.Extension<$type$> $name$ =\n" - " com.google.protobuf.nano.Extension.create($number$,\n" - " new com.google.protobuf.nano.Extension.TypeLiteral<$type$>(){});\n"); - } + printer->Print("\n"); + PrintFieldComment(printer, descriptor_); + printer->Print(variables_, + "public static final com.google.protobuf.nano.Extension<\n" + " $extends$,\n" + " $class$> $name$ =\n" + " com.google.protobuf.nano.Extension.create$repeated$$ext_type$(\n" + " com.google.protobuf.nano.Extension.$type$,\n" + " $class$.class,\n" + " $tag_params$);\n"); } } // namespace javanano diff --git a/src/google/protobuf/compiler/javanano/javanano_helpers.cc b/src/google/protobuf/compiler/javanano/javanano_helpers.cc index b3bedcb..893cdde 100644 --- a/src/google/protobuf/compiler/javanano/javanano_helpers.cc +++ b/src/google/protobuf/compiler/javanano/javanano_helpers.cc @@ -264,6 +264,22 @@ string FieldDefaultConstantName(const FieldDescriptor *field) { return "_" + RenameJavaKeywords(UnderscoresToCamelCase(field)) + "Default"; } +void PrintFieldComment(io::Printer* printer, const FieldDescriptor* field) { + // We don't want to print group bodies so we cut off after the first line + // (the second line for extensions). + string def = field->DebugString(); + string::size_type first_line_end = def.find_first_of('\n'); + printer->Print("// $def$\n", + "def", def.substr(0, first_line_end)); + if (field->is_extension()) { + string::size_type second_line_start = first_line_end + 1; + string::size_type second_line_length = + def.find('\n', second_line_start) - second_line_start; + printer->Print("// $def$\n", + "def", def.substr(second_line_start, second_line_length)); + } +} + JavaType GetJavaType(FieldDescriptor::Type field_type) { switch (field_type) { case FieldDescriptor::TYPE_INT32: @@ -310,7 +326,27 @@ JavaType GetJavaType(FieldDescriptor::Type field_type) { return JAVATYPE_INT; } -const char* BoxedPrimitiveTypeName(JavaType type) { +string PrimitiveTypeName(JavaType type) { + switch (type) { + case JAVATYPE_INT : return "int"; + case JAVATYPE_LONG : return "long"; + case JAVATYPE_FLOAT : return "float"; + case JAVATYPE_DOUBLE : return "double"; + case JAVATYPE_BOOLEAN: return "boolean"; + case JAVATYPE_STRING : return "java.lang.String"; + case JAVATYPE_BYTES : return "byte[]"; + case JAVATYPE_ENUM : return "int"; + case JAVATYPE_MESSAGE: return NULL; + + // No default because we want the compiler to complain if any new + // JavaTypes are added. + } + + GOOGLE_LOG(FATAL) << "Can't get here."; + return NULL; +} + +string BoxedPrimitiveTypeName(JavaType type) { switch (type) { case JAVATYPE_INT : return "java.lang.Integer"; case JAVATYPE_LONG : return "java.lang.Long"; diff --git a/src/google/protobuf/compiler/javanano/javanano_helpers.h b/src/google/protobuf/compiler/javanano/javanano_helpers.h index 753a4bd..886bff8 100644 --- a/src/google/protobuf/compiler/javanano/javanano_helpers.h +++ b/src/google/protobuf/compiler/javanano/javanano_helpers.h @@ -39,6 +39,7 @@ #include #include #include +#include namespace google { namespace protobuf { @@ -111,6 +112,9 @@ string FieldConstantName(const FieldDescriptor *field); string FieldDefaultConstantName(const FieldDescriptor *field); +// Print the field's proto-syntax definition as a comment. +void PrintFieldComment(io::Printer* printer, const FieldDescriptor* field); + enum JavaType { JAVATYPE_INT, JAVATYPE_LONG, @@ -129,10 +133,12 @@ inline JavaType GetJavaType(const FieldDescriptor* field) { return GetJavaType(field->type()); } +string PrimitiveTypeName(JavaType type); + // Get the fully-qualified class name for a boxed primitive type, e.g. // "java.lang.Integer" for JAVATYPE_INT. Returns NULL for enum and message // types. -const char* BoxedPrimitiveTypeName(JavaType type); +string BoxedPrimitiveTypeName(JavaType type); string EmptyArrayName(const Params& params, const FieldDescriptor* field); diff --git a/src/google/protobuf/compiler/javanano/javanano_message.cc b/src/google/protobuf/compiler/javanano/javanano_message.cc index c09670a..008bec2 100644 --- a/src/google/protobuf/compiler/javanano/javanano_message.cc +++ b/src/google/protobuf/compiler/javanano/javanano_message.cc @@ -54,14 +54,6 @@ using internal::WireFormatLite; namespace { -void PrintFieldComment(io::Printer* printer, const FieldDescriptor* field) { - // Print the field's proto-syntax definition as a comment. We don't want to - // print group bodies so we cut off after the first line. - string def = field->DebugString(); - printer->Print("// $def$\n", - "def", def.substr(0, def.find_first_of('\n'))); -} - struct FieldOrderingByNumber { inline bool operator()(const FieldDescriptor* a, const FieldDescriptor* b) const { @@ -82,13 +74,6 @@ const FieldDescriptor** SortFieldsByNumber(const Descriptor* descriptor) { return fields; } -// Get an identifier that uniquely identifies this type within the file. -// This is used to declare static variables related to this type at the -// outermost file scope. -string UniqueFileScopeIdentifier(const Descriptor* descriptor) { - return "static_" + StringReplace(descriptor->full_name(), ".", "_", true); -} - } // namespace // =================================================================== @@ -149,7 +134,8 @@ void MessageGenerator::Generate(io::Printer* printer) { } if (params_.store_unknown_fields()) { printer->Print( - " com.google.protobuf.nano.ExtendableMessageNano {\n"); + " com.google.protobuf.nano.ExtendableMessageNano<$classname$> {\n", + "classname", descriptor_->name()); } else { printer->Print( " com.google.protobuf.nano.MessageNano {\n"); @@ -285,22 +271,20 @@ void MessageGenerator::Generate(io::Printer* printer) { void MessageGenerator:: GenerateMessageSerializationMethods(io::Printer* printer) { + // Rely on the parent implementations of writeTo() and getSerializedSize() + // if there are no fields to serialize in this message. + if (descriptor_->field_count() == 0) { + return; + } + scoped_array sorted_fields( SortFieldsByNumber(descriptor_)); - // writeTo only throws an exception if it contains one or more fields to write - if (descriptor_->field_count() > 0 || params_.store_unknown_fields()) { - printer->Print( - "\n" - "@Override\n" - "public void writeTo(com.google.protobuf.nano.CodedOutputByteBufferNano output)\n" - " throws java.io.IOException {\n"); - } else { - printer->Print( - "\n" - "@Override\n" - "public void writeTo(com.google.protobuf.nano.CodedOutputByteBufferNano output) {\n"); - } + printer->Print( + "\n" + "@Override\n" + "public void writeTo(com.google.protobuf.nano.CodedOutputByteBufferNano output)\n" + " throws java.io.IOException {\n"); printer->Indent(); // Output the fields in sorted order @@ -308,36 +292,31 @@ GenerateMessageSerializationMethods(io::Printer* printer) { GenerateSerializeOneField(printer, sorted_fields[i]); } - // Write unknown fields. - if (params_.store_unknown_fields()) { - printer->Print( - "com.google.protobuf.nano.WireFormatNano.writeUnknownFields(\n" - " unknownFieldData, output);\n"); - } + // The parent implementation will write any unknown fields if necessary. + printer->Print( + "super.writeTo(output);\n"); printer->Outdent(); printer->Print("}\n"); - // Rely on the parent implementation of getSerializedSize if there are no fields to - // serialize in this MessageNano. - if (descriptor_->field_count() != 0) { - printer->Print( - "\n" - "@Override\n" - "public int getSerializedSize() {\n" - " int size = super.getSerializedSize();\n"); - printer->Indent(); - - for (int i = 0; i < descriptor_->field_count(); i++) { - field_generators_.get(sorted_fields[i]).GenerateSerializedSizeCode(printer); - } + // The parent implementation will get the serialized size for unknown + // fields if necessary. + printer->Print( + "\n" + "@Override\n" + "public int getSerializedSize() {\n" + " int size = super.getSerializedSize();\n"); + printer->Indent(); - printer->Outdent(); - printer->Print( - " cachedSize = size;\n" - " return size;\n" - "}\n"); + for (int i = 0; i < descriptor_->field_count(); i++) { + field_generators_.get(sorted_fields[i]).GenerateSerializedSizeCode(printer); } + + printer->Outdent(); + printer->Print( + " cachedSize = size;\n" + " return size;\n" + "}\n"); } void MessageGenerator::GenerateMergeFromMethods(io::Printer* printer) { @@ -371,12 +350,7 @@ void MessageGenerator::GenerateMergeFromMethods(io::Printer* printer) { printer->Indent(); if (params_.store_unknown_fields()) { printer->Print( - "if (unknownFieldData == null) {\n" - " unknownFieldData =\n" - " new java.util.ArrayList();\n" - "}\n" - "if (!com.google.protobuf.nano.WireFormatNano.storeUnknownField(\n" - " unknownFieldData, input, tag)) {\n" + "if (!storeUnknownField(input, tag)) {\n" " return this;\n" "}\n"); } else { diff --git a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc index 3428f69..06c0b8b 100644 --- a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc @@ -54,26 +54,6 @@ using internal::WireFormatLite; namespace { -const char* PrimitiveTypeName(JavaType type) { - switch (type) { - case JAVATYPE_INT : return "int"; - case JAVATYPE_LONG : return "long"; - case JAVATYPE_FLOAT : return "float"; - case JAVATYPE_DOUBLE : return "double"; - case JAVATYPE_BOOLEAN: return "boolean"; - case JAVATYPE_STRING : return "java.lang.String"; - case JAVATYPE_BYTES : return "byte[]"; - case JAVATYPE_ENUM : return NULL; - case JAVATYPE_MESSAGE: return NULL; - - // No default because we want the compiler to complain if any new - // JavaTypes are added. - } - - GOOGLE_LOG(FATAL) << "Can't get here."; - return NULL; -} - bool IsReferenceType(JavaType type) { switch (type) { case JAVATYPE_INT : return false; diff --git a/src/google/protobuf/unittest_extension_nano.proto b/src/google/protobuf/unittest_extension_nano.proto index 104cfa7..0a775f4 100644 --- a/src/google/protobuf/unittest_extension_nano.proto +++ b/src/google/protobuf/unittest_extension_nano.proto @@ -18,31 +18,14 @@ message AnotherMessage { optional bool value = 2; } -extend ExtendableMessage { - optional string some_string = 10; - optional int32 some_int = 11; - optional int64 some_long = 12; - optional float some_float = 13; - optional double some_double = 14; - optional bool some_bool = 15; - optional AnEnum some_enum = 16; - optional AnotherMessage some_message = 17; - repeated string some_repeated_string = 18; - repeated int32 some_repeated_int = 19; - repeated int64 some_repeated_long = 20; - repeated float some_repeated_float = 21; - repeated double some_repeated_double = 22; - repeated bool some_repeated_bool = 23; - repeated AnEnum some_repeated_enum = 24; - repeated AnotherMessage some_repeated_message = 25; -} - message ContainerMessage { extend ExtendableMessage { optional bool another_thing = 100; } } +// For testNanoOptionalGroupWithUnknownFieldsEnabled; +// not part of the extensions tests. message MessageWithGroup { optional group Group = 1 { optional int32 a = 2; diff --git a/src/google/protobuf/unittest_extension_packed_nano.proto b/src/google/protobuf/unittest_extension_packed_nano.proto new file mode 100644 index 0000000..3586de7 --- /dev/null +++ b/src/google/protobuf/unittest_extension_packed_nano.proto @@ -0,0 +1,29 @@ +syntax = "proto2"; + +option java_multiple_files = true; +option java_package = "com.google.protobuf.nano"; + +import "google/protobuf/unittest_extension_nano.proto"; + +// Must be compiled separately due to extension number reuse. +// The reuse is deliberate, for testing wire compatibility. + +message PackedExtensions { + extend ExtendableMessage { + repeated int32 packed_int32 = 10 [ packed = true ]; + repeated uint32 packed_uint32 = 11 [ packed = true ]; + repeated sint32 packed_sint32 = 12 [ packed = true ]; + repeated int64 packed_int64 = 13 [ packed = true ]; + repeated uint64 packed_uint64 = 14 [ packed = true ]; + repeated sint64 packed_sint64 = 15 [ packed = true ]; + repeated fixed32 packed_fixed32 = 16 [ packed = true ]; + repeated sfixed32 packed_sfixed32 = 17 [ packed = true ]; + repeated fixed64 packed_fixed64 = 18 [ packed = true ]; + repeated sfixed64 packed_sfixed64 = 19 [ packed = true ]; + repeated bool packed_bool = 20 [ packed = true ]; + repeated float packed_float = 21 [ packed = true ]; + repeated double packed_double = 22 [ packed = true ]; + repeated AnEnum packed_enum = 23 [ packed = true ]; + // Non-packable types omitted. + } +} diff --git a/src/google/protobuf/unittest_extension_repeated_nano.proto b/src/google/protobuf/unittest_extension_repeated_nano.proto new file mode 100644 index 0000000..546c2df --- /dev/null +++ b/src/google/protobuf/unittest_extension_repeated_nano.proto @@ -0,0 +1,34 @@ +syntax = "proto2"; + +option java_multiple_files = true; +option java_package = "com.google.protobuf.nano"; + +import "google/protobuf/unittest_extension_nano.proto"; + +// Must be compiled separately due to extension number reuse. +// The reuse is deliberate, for testing wire compatibility. + +message RepeatedExtensions { + extend ExtendableMessage { + repeated int32 repeated_int32 = 10; + repeated uint32 repeated_uint32 = 11; + repeated sint32 repeated_sint32 = 12; + repeated int64 repeated_int64 = 13; + repeated uint64 repeated_uint64 = 14; + repeated sint64 repeated_sint64 = 15; + repeated fixed32 repeated_fixed32 = 16; + repeated sfixed32 repeated_sfixed32 = 17; + repeated fixed64 repeated_fixed64 = 18; + repeated sfixed64 repeated_sfixed64 = 19; + repeated bool repeated_bool = 20; + repeated float repeated_float = 21; + repeated double repeated_double = 22; + repeated AnEnum repeated_enum = 23; + repeated string repeated_string = 24; + repeated bytes repeated_bytes = 25; + repeated AnotherMessage repeated_message = 26; + repeated group RepeatedGroup = 27 { + optional int32 a = 1; + } + } +} diff --git a/src/google/protobuf/unittest_extension_singular_nano.proto b/src/google/protobuf/unittest_extension_singular_nano.proto new file mode 100644 index 0000000..35d9e6e --- /dev/null +++ b/src/google/protobuf/unittest_extension_singular_nano.proto @@ -0,0 +1,34 @@ +syntax = "proto2"; + +option java_multiple_files = true; +option java_package = "com.google.protobuf.nano"; + +import "google/protobuf/unittest_extension_nano.proto"; + +// Must be compiled separately due to extension number reuse. +// The reuse is deliberate, for testing wire compatibility. + +message SingularExtensions { + extend ExtendableMessage { + optional int32 some_int32 = 10; + optional uint32 some_uint32 = 11; + optional sint32 some_sint32 = 12; + optional int64 some_int64 = 13; + optional uint64 some_uint64 = 14; + optional sint64 some_sint64 = 15; + optional fixed32 some_fixed32 = 16; + optional sfixed32 some_sfixed32 = 17; + optional fixed64 some_fixed64 = 18; + optional sfixed64 some_sfixed64 = 19; + optional bool some_bool = 20; + optional float some_float = 21; + optional double some_double = 22; + optional AnEnum some_enum = 23; + optional string some_string = 24; + optional bytes some_bytes = 25; + optional AnotherMessage some_message = 26; + optional group SomeGroup = 27 { + optional int32 a = 1; + } + } +} -- cgit v1.1