diff --git a/BUILD b/BUILD index 8ab4cb413e..daad6d81f5 100644 --- a/BUILD +++ b/BUILD @@ -486,7 +486,6 @@ RELATIVE_LITE_TEST_PROTOS = [ "google/protobuf/unittest_import_lite.proto", "google/protobuf/unittest_import_public_lite.proto", "google/protobuf/unittest_lite.proto", - "google/protobuf/unittest_no_arena_lite.proto", ] LITE_TEST_PROTOS = ["src/" + s for s in RELATIVE_LITE_TEST_PROTOS] @@ -513,8 +512,6 @@ RELATIVE_TEST_PROTOS = [ "google/protobuf/unittest_lite_imports_nonlite.proto", "google/protobuf/unittest_mset.proto", "google/protobuf/unittest_mset_wire_format.proto", - "google/protobuf/unittest_no_arena.proto", - "google/protobuf/unittest_no_arena_import.proto", "google/protobuf/unittest_no_field_presence.proto", "google/protobuf/unittest_no_generic_services.proto", "google/protobuf/unittest_optimize_for.proto", diff --git a/CHANGES.txt b/CHANGES.txt index 317930626e..6234f1635d 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -44,6 +44,7 @@ * Mark java enum _VALUE constants as @Deprecated if the enum field is deprecated * reduce size for enums with allow_alias set to true. * Sort map fields alphabetically by the field's key when printing textproto. + * Fixed a bug in map sorting that appeared in -rc1 and -rc2 (#7508). * TextFormat.merge() handles Any as top level type. * Throw a descriptive IllegalArgumentException when calling getValueDescriptor() on enum special value UNRECOGNIZED instead of diff --git a/Protobuf.podspec b/Protobuf.podspec index 5fe982a7f7..5ff3597e0e 100644 --- a/Protobuf.podspec +++ b/Protobuf.podspec @@ -5,7 +5,7 @@ # dependent projects use the :git notation to refer to the library. Pod::Spec.new do |s| s.name = 'Protobuf' - s.version = '3.12.0-rc2' + s.version = '3.12.0' s.summary = 'Protocol Buffers v.3 runtime library for Objective-C.' s.homepage = 'https://github.com/protocolbuffers/protobuf' s.license = '3-Clause BSD License' diff --git a/csharp/Google.Protobuf.Tools.nuspec b/csharp/Google.Protobuf.Tools.nuspec index e2a46ce4fe..d28a44e0ef 100644 --- a/csharp/Google.Protobuf.Tools.nuspec +++ b/csharp/Google.Protobuf.Tools.nuspec @@ -5,7 +5,7 @@ Google Protocol Buffers tools Tools for Protocol Buffers - Google's data interchange format. See project site for more info. - 3.12.0-rc2 + 3.12.0 Google Inc. protobuf-packages https://github.com/protocolbuffers/protobuf/blob/master/LICENSE diff --git a/csharp/src/Google.Protobuf/Google.Protobuf.csproj b/csharp/src/Google.Protobuf/Google.Protobuf.csproj index 5c5b4e7b0b..3ceb832cc7 100644 --- a/csharp/src/Google.Protobuf/Google.Protobuf.csproj +++ b/csharp/src/Google.Protobuf/Google.Protobuf.csproj @@ -4,7 +4,7 @@ C# runtime library for Protocol Buffers - Google's data interchange format. Copyright 2015, Google Inc. Google Protocol Buffers - 3.12.0-rc2 + 3.12.0 7.2 Google Inc. diff --git a/docs/implementing_proto3_presence.md b/docs/implementing_proto3_presence.md index 08f9c51321..fae2eb589a 100644 --- a/docs/implementing_proto3_presence.md +++ b/docs/implementing_proto3_presence.md @@ -5,6 +5,8 @@ proto3. Proto3 optional fields track presence like in proto2. For background information about what presence tracking means, please see [docs/field_presence](field_presence.md). +## Document Summary + This document is targeted at developers who own or maintain protobuf code generators. All code generators will need to be updated to support proto3 optional fields. First-party code generators developed by Google are being @@ -14,6 +16,7 @@ independently by their authors. This includes: - implementations of Protocol Buffers for other languges. - alternate implementations of Protocol Buffers that target specialized use cases. +- RPC code generators that create generated APIs for service calls. - code generators that implement some utility code on top of protobuf generated classes. @@ -21,6 +24,53 @@ While this document speaks in terms of "code generators", these same principles apply to implementations that dynamically generate a protocol buffer API "on the fly", directly from a descriptor, in languages that support this kind of usage. +## Background + +Presence tracking was added to proto3 in response to user feedback, both from +inside Google and [from open-source +users](https://github.com/protocolbuffers/protobuf/issues/1606). The [proto3 +wrapper +types](https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/wrappers.proto) +were previously the only supported presence mechanism for proto3. Users have +pointed to both efficiency and usability issues with the wrapper types. + +Presence in proto3 uses exactly the same syntax and semantics as in proto2. +Proto3 Fields marked `optional` will track presence like proto2, while fields +without any label (known as "singular fields"), will continue to omit presence +information. The `optional` keyword was chosen to minimize differences with +proto2. + +Unfortunately, for the current descriptor protos and `Descriptor` API (as of +3.11.4) it is not possible to use the same representation as proto2. Proto3 +descriptors already use `LABEL_OPTIONAL` for proto3 singular fields, which do +not track presence. There is a lot of existing code that reflects over proto3 +protos and assumes that `LABEL_OPTIONAL` in proto3 means "no presence." Changing +the semantics now would be risky, since old software would likely drop proto3 +presence information, which would be a data loss bug. + +To minimize this risk we chose a descriptor representation that is semantically +compatible with existing proto3 reflection. Every proto3 optional field is +placed into a one-field `oneof`. We call this a "synthetic" oneof, as it was not +present in the source `.proto` file. + +Since oneof fields in proto3 already track presence, existing proto3 +reflection-based algorithms should correctly preserve presence for proto3 +optional fields with no code changes. For example, the JSON and TextFormat +parsers/serializers in C++ and Java did not require any changes to support +proto3 presence. This is the major benefit of synthetic oneofs. + +This design does leave some cruft in descriptors. Synthetic oneofs are a +compatibility measure that we can hopefully clean up in the future. For now +though, it is important to preserve them across different descriptor formats and +APIs. It is never safe to drop synthetic oneofs from a proto schema. Code +generators can (and should) skip synthetic oneofs when generating a user-facing +API or user-facing documentation. But for any schema representation that is +consumed programmatically, it is important to keep the synthetic oneofs around. + +In APIs it can be helpful to offer separate accessors that refer to "real" +oneofs (see [API Changes](#api-changes) below). This is a convenient way to omit +synthetic oneofs in code generators. + ## Updating a Code Generator When a user adds an `optional` field to proto3, this is internally rewritten as diff --git a/editors/protobuf-mode.el b/editors/protobuf-mode.el index d3bdcded70..f73d4b2b45 100644 --- a/editors/protobuf-mode.el +++ b/editors/protobuf-mode.el @@ -1,4 +1,4 @@ -;;; protobuf-mode.el --- major mode for editing protocol buffers. +;;; protobuf-mode.el --- major mode for editing protocol buffers. -*- lexical-binding: t; -*- ;; Author: Alexandre Vassalotti ;; Created: 23-Apr-2009 diff --git a/java/bom/pom.xml b/java/bom/pom.xml index 04bd3f5bda..6fca4aa45b 100644 --- a/java/bom/pom.xml +++ b/java/bom/pom.xml @@ -4,7 +4,7 @@ com.google.protobuf protobuf-bom - 3.12.0-rc-2 + 3.12.0 pom Protocol Buffers [BOM] diff --git a/java/core/pom.xml b/java/core/pom.xml index 5fb5045955..1bbf90e0e9 100644 --- a/java/core/pom.xml +++ b/java/core/pom.xml @@ -4,7 +4,7 @@ com.google.protobuf protobuf-parent - 3.12.0-rc-2 + 3.12.0 protobuf-java diff --git a/java/core/src/main/java/com/google/protobuf/GeneratedMessageLite.java b/java/core/src/main/java/com/google/protobuf/GeneratedMessageLite.java index 8456713fa0..07b008004c 100644 --- a/java/core/src/main/java/com/google/protobuf/GeneratedMessageLite.java +++ b/java/core/src/main/java/com/google/protobuf/GeneratedMessageLite.java @@ -234,7 +234,7 @@ public abstract class GeneratedMessageLite< * It doesn't use or modify any memoized value. *
  • {@code GET_MEMOIZED_IS_INITIALIZED} returns the memoized {@code isInitialized} byte * value. - *
  • {@code SET_MEMOIZED_IS_INITIALIZED} sets the memoized {@code isInitilaized} byte value to + *
  • {@code SET_MEMOIZED_IS_INITIALIZED} sets the memoized {@code isInitialized} byte value to * 1 if the first parameter is not null, or to 0 if the first parameter is null. *
  • {@code NEW_BUILDER} returns a {@code BuilderType} instance. * @@ -461,7 +461,7 @@ public abstract class GeneratedMessageLite< throws IOException { copyOnWrite(); try { - // TODO(yilunchong): Try to make input with type CodedInpuStream.ArrayDecoder use + // TODO(yilunchong): Try to make input with type CodedInputStream.ArrayDecoder use // fast path. Protobuf.getInstance().schemaFor(instance).mergeFrom( instance, CodedInputStreamReader.forCodedInput(input), extensionRegistry); diff --git a/java/core/src/main/java/com/google/protobuf/TextFormat.java b/java/core/src/main/java/com/google/protobuf/TextFormat.java index 673343d4ea..bec9623fe8 100644 --- a/java/core/src/main/java/com/google/protobuf/TextFormat.java +++ b/java/core/src/main/java/com/google/protobuf/TextFormat.java @@ -490,27 +490,11 @@ public final class TextFormat { } switch (fieldType) { case BOOLEAN: - boolean aBoolean = (boolean) getKey(); - boolean bBoolean = (boolean) b.getKey(); - if (aBoolean == bBoolean) { - return 0; - } else if (aBoolean) { - return 1; - } else { - return -1; - } + return Boolean.compare((boolean) getKey(), (boolean) b.getKey()); case LONG: - long aLong = (long) getKey(); - long bLong = (long) b.getKey(); - if (aLong < bLong) { - return -1; - } else if (aLong > bLong) { - return 1; - } else { - return 0; - } + return Long.compare((long) getKey(), (long) b.getKey()); case INT: - return (int) getKey() - (int) b.getKey(); + return Integer.compare((int) getKey(), (int) b.getKey()); case STRING: String aString = (String) getKey(); String bString = (String) b.getKey(); diff --git a/java/core/src/test/java/com/google/protobuf/TextFormatTest.java b/java/core/src/test/java/com/google/protobuf/TextFormatTest.java index 6ca3ae111f..a9a884e196 100644 --- a/java/core/src/test/java/com/google/protobuf/TextFormatTest.java +++ b/java/core/src/test/java/com/google/protobuf/TextFormatTest.java @@ -1404,6 +1404,27 @@ public class TextFormatTest extends TestCase { } } + public void testMapDuplicateKeys() throws Exception { + String input = + "int32_to_int32_field: {\n" + + " key: 1\n" + + " value: 1\n" + + "}\n" + + "int32_to_int32_field: {\n" + + " key: -2147483647\n" + + " value: 5\n" + + "}\n" + + "int32_to_int32_field: {\n" + + " key: 1\n" + + " value: -1\n" + + "}\n"; + TestMap msg = TextFormat.parse(input, TestMap.class); + int i1 = msg.getInt32ToInt32Field().get(1); + TestMap msg2 = TextFormat.parse(msg.toString(), TestMap.class); + int i2 = msg2.getInt32ToInt32Field().get(1); + assertEquals(i1, i2); + } + public void testMapShortForm() throws Exception { String text = "string_to_int32_field [{ key: 'x' value: 10 }, { key: 'y' value: 20 }]\n" diff --git a/java/lite/pom.xml b/java/lite/pom.xml index 6ce28ee5e1..c5901a2c3f 100644 --- a/java/lite/pom.xml +++ b/java/lite/pom.xml @@ -4,7 +4,7 @@ com.google.protobuf protobuf-parent - 3.12.0-rc-2 + 3.12.0 protobuf-javalite diff --git a/java/pom.xml b/java/pom.xml index eb15314247..706e43dd8b 100644 --- a/java/pom.xml +++ b/java/pom.xml @@ -4,7 +4,7 @@ com.google.protobuf protobuf-parent - 3.12.0-rc-2 + 3.12.0 pom Protocol Buffers [Parent] diff --git a/java/util/pom.xml b/java/util/pom.xml index b7d96766f7..5d2cbc0081 100644 --- a/java/util/pom.xml +++ b/java/util/pom.xml @@ -4,7 +4,7 @@ com.google.protobuf protobuf-parent - 3.12.0-rc-2 + 3.12.0 protobuf-java-util diff --git a/js/experimental/runtime/internal/checks.js b/js/experimental/runtime/internal/checks.js index 369c6af6ef..3d1af548f5 100644 --- a/js/experimental/runtime/internal/checks.js +++ b/js/experimental/runtime/internal/checks.js @@ -2,7 +2,7 @@ * @fileoverview Proto internal runtime checks. * * Checks are grouped into different severity, see: - * http://g3doc/javascript/protobuf/README.md#configurable-check-support-in-protocol-buffers + * http://g3doc/third_party/protobuf/javascript/README.md#configurable-check-support-in-protocol-buffers * * Checks are also grouped into different sections: * - CHECK_BOUNDS: @@ -23,7 +23,7 @@ const WireType = goog.require('protobuf.binary.WireType'); // // See -// http://g3doc/javascript/protobuf/README.md#configurable-check-support-in-protocol-buffers +// http://g3doc/third_party/protobuf/javascript/README.md#configurable-check-support-in-protocol-buffers // /** @define{string} */ const CHECK_LEVEL_DEFINE = goog.define('protobuf.defines.CHECK_LEVEL', ''); diff --git a/kokoro/macos/php7.4_mac/build.sh b/kokoro/macos/php7.3_mac/build.sh similarity index 89% rename from kokoro/macos/php7.4_mac/build.sh rename to kokoro/macos/php7.3_mac/build.sh index 98c82d4bb3..2d2f679da2 100755 --- a/kokoro/macos/php7.4_mac/build.sh +++ b/kokoro/macos/php7.3_mac/build.sh @@ -8,4 +8,4 @@ cd $(dirname $0)/../../.. # Prepare worker environment to run tests source kokoro/macos/prepare_build_macos_rc -./tests.sh php7.4_mac +./tests.sh php7.3_mac diff --git a/kokoro/macos/php7.4_mac/continuous.cfg b/kokoro/macos/php7.3_mac/continuous.cfg similarity index 65% rename from kokoro/macos/php7.4_mac/continuous.cfg rename to kokoro/macos/php7.3_mac/continuous.cfg index 5b2d6fde98..9a717451d3 100644 --- a/kokoro/macos/php7.4_mac/continuous.cfg +++ b/kokoro/macos/php7.3_mac/continuous.cfg @@ -1,5 +1,5 @@ # Config file for running tests in Kokoro # Location of the build script in repository -build_file: "protobuf/kokoro/macos/php7.4_mac/build.sh" +build_file: "protobuf/kokoro/macos/php7.3_mac/build.sh" timeout_mins: 1440 diff --git a/kokoro/macos/php7.4_mac/presubmit.cfg b/kokoro/macos/php7.3_mac/presubmit.cfg similarity index 65% rename from kokoro/macos/php7.4_mac/presubmit.cfg rename to kokoro/macos/php7.3_mac/presubmit.cfg index 5b2d6fde98..9a717451d3 100644 --- a/kokoro/macos/php7.4_mac/presubmit.cfg +++ b/kokoro/macos/php7.3_mac/presubmit.cfg @@ -1,5 +1,5 @@ # Config file for running tests in Kokoro # Location of the build script in repository -build_file: "protobuf/kokoro/macos/php7.4_mac/build.sh" +build_file: "protobuf/kokoro/macos/php7.3_mac/build.sh" timeout_mins: 1440 diff --git a/kokoro/release/ruby/linux/ruby/ruby_build.sh b/kokoro/release/ruby/linux/ruby/ruby_build.sh index 95f1dea906..f8ae9623f2 100755 --- a/kokoro/release/ruby/linux/ruby/ruby_build.sh +++ b/kokoro/release/ruby/linux/ruby/ruby_build.sh @@ -12,7 +12,7 @@ fi umask 0022 pushd ruby gem install bundler -v 2.1.4 -bundle install && bundle exec rake gem:native +bundle update && bundle exec rake gem:native ls pkg mv pkg/* $ARTIFACT_DIR popd diff --git a/kokoro/release/ruby/macos/ruby/ruby_build.sh b/kokoro/release/ruby/macos/ruby/ruby_build.sh index 9fc42b13eb..7f6c18fe8e 100755 --- a/kokoro/release/ruby/macos/ruby/ruby_build.sh +++ b/kokoro/release/ruby/macos/ruby/ruby_build.sh @@ -11,7 +11,7 @@ fi umask 0022 pushd ruby -bundle install && bundle exec rake gem:native +bundle update && bundle exec rake gem:native ls pkg mv pkg/* $ARTIFACT_DIR popd diff --git a/kokoro/release/ruby/macos/ruby/ruby_build_environment.sh b/kokoro/release/ruby/macos/ruby/ruby_build_environment.sh index 7ff1ce5696..880c23390c 100755 --- a/kokoro/release/ruby/macos/ruby/ruby_build_environment.sh +++ b/kokoro/release/ruby/macos/ruby/ruby_build_environment.sh @@ -63,7 +63,7 @@ set +x rvm use 2.5.0 set -x ruby --version | grep 'ruby 2.5.0' -for v in 2.6.0 2.5.1 ; do +for v in 2.6.0 2.5.1 2.4.0 2.3.0; do ccache -c rake -f "$CROSS_RUBY" cross-ruby VERSION="$v" HOST=x86_64-darwin11 MAKE="$MAKE" done diff --git a/objectivec/DevTools/compile_testing_protos.sh b/objectivec/DevTools/compile_testing_protos.sh index d04c5c522b..021e03d0ed 100755 --- a/objectivec/DevTools/compile_testing_protos.sh +++ b/objectivec/DevTools/compile_testing_protos.sh @@ -42,8 +42,6 @@ CORE_PROTO_FILES=( src/google/protobuf/unittest_lite.proto src/google/protobuf/unittest_mset.proto src/google/protobuf/unittest_mset_wire_format.proto - src/google/protobuf/unittest_no_arena.proto - src/google/protobuf/unittest_no_arena_import.proto src/google/protobuf/unittest_no_generic_services.proto src/google/protobuf/unittest_optimize_for.proto src/google/protobuf/unittest.proto diff --git a/objectivec/Tests/GPBUnittestProtos.m b/objectivec/Tests/GPBUnittestProtos.m index 756bd99ef7..1c6eddfbac 100644 --- a/objectivec/Tests/GPBUnittestProtos.m +++ b/objectivec/Tests/GPBUnittestProtos.m @@ -56,8 +56,6 @@ #import "google/protobuf/UnittestLite.pbobjc.m" #import "google/protobuf/UnittestMset.pbobjc.m" #import "google/protobuf/UnittestMsetWireFormat.pbobjc.m" -#import "google/protobuf/UnittestNoArena.pbobjc.m" -#import "google/protobuf/UnittestNoArenaImport.pbobjc.m" #import "google/protobuf/UnittestNoGenericServices.pbobjc.m" #import "google/protobuf/UnittestObjc.pbobjc.m" #import "google/protobuf/UnittestObjcStartup.pbobjc.m" diff --git a/php/ext/google/protobuf/package.xml b/php/ext/google/protobuf/package.xml index 4b9066a32d..687c43de0f 100644 --- a/php/ext/google/protobuf/package.xml +++ b/php/ext/google/protobuf/package.xml @@ -10,15 +10,15 @@ protobuf-opensource@google.com yes - 2020-05-12 - + 2020-05-15 + - 3.12.0RC2 + 3.12.0 3.12.0 - beta - beta + stable + stable 3-Clause BSD License GA release. @@ -557,5 +557,19 @@ G A release. 3-Clause BSD License GA release. + + + 3.12.0 + 3.12.0 + + + stable + stable + + 2020-05-15 + + 3-Clause BSD License + GA release. + diff --git a/php/ext/google/protobuf/protobuf.h b/php/ext/google/protobuf/protobuf.h index bc293b8030..4f40ec224b 100644 --- a/php/ext/google/protobuf/protobuf.h +++ b/php/ext/google/protobuf/protobuf.h @@ -37,7 +37,7 @@ #include "upb.h" #define PHP_PROTOBUF_EXTNAME "protobuf" -#define PHP_PROTOBUF_VERSION "3.12.0RC2" +#define PHP_PROTOBUF_VERSION "3.12.0" #define MAX_LENGTH_OF_INT64 20 #define SIZEOF_INT64 8 diff --git a/protobuf.bzl b/protobuf.bzl index 8d67620f83..027937f89c 100644 --- a/protobuf.bzl +++ b/protobuf.bzl @@ -76,18 +76,17 @@ def _RelativeOutputPath(path, include, dest = ""): def _proto_gen_impl(ctx): """General implementation for generating protos""" srcs = ctx.files.srcs - deps = [] - deps += ctx.files.srcs + deps = depset(direct=ctx.files.srcs) source_dir = _SourceDir(ctx) gen_dir = _GenDir(ctx).rstrip("/") if source_dir: - import_flags = ["-I" + source_dir, "-I" + gen_dir] + import_flags = depset(direct=["-I" + source_dir, "-I" + gen_dir]) else: - import_flags = ["-I."] + import_flags = depset(direct=["-I."]) for dep in ctx.attr.deps: - import_flags += dep.proto.import_flags - deps += dep.proto.deps + import_flags = depset(transitive=[import_flags, depset(direct=dep.proto.import_flags)]) + deps = depset(transitive=[deps, depset(direct=dep.proto.deps)]) if not ctx.attr.gen_cc and not ctx.attr.gen_py and not ctx.executable.plugin: return struct( @@ -104,7 +103,7 @@ def _proto_gen_impl(ctx): in_gen_dir = src.root.path == gen_dir if in_gen_dir: import_flags_real = [] - for f in depset(import_flags).to_list(): + for f in import_flags.to_list(): path = f.replace("-I", "") import_flags_real.append("-I$(realpath -s %s)" % path) @@ -119,7 +118,7 @@ def _proto_gen_impl(ctx): outs.extend(_PyOuts([src.basename], use_grpc_plugin = use_grpc_plugin)) outs = [ctx.actions.declare_file(out, sibling = src) for out in outs] - inputs = [src] + deps + inputs = [src] + deps.to_list() tools = [ctx.executable.protoc] if ctx.executable.plugin: plugin = ctx.executable.plugin @@ -142,7 +141,7 @@ def _proto_gen_impl(ctx): inputs = inputs, tools = tools, outputs = outs, - arguments = args + import_flags + [src.path], + arguments = args + import_flags.to_list() + [src.path], executable = ctx.executable.protoc, mnemonic = "ProtoCompile", use_default_shell_env = True, diff --git a/protoc-artifacts/pom.xml b/protoc-artifacts/pom.xml index ae72dbbfa6..c64afb1d7d 100644 --- a/protoc-artifacts/pom.xml +++ b/protoc-artifacts/pom.xml @@ -8,7 +8,7 @@ com.google.protobuf protoc - 3.12.0-rc-2 + 3.12.0 pom Protobuf Compiler diff --git a/python/google/protobuf/internal/enum_type_wrapper.py b/python/google/protobuf/internal/enum_type_wrapper.py index eeb2dae90f..c8e10137b9 100644 --- a/python/google/protobuf/internal/enum_type_wrapper.py +++ b/python/google/protobuf/internal/enum_type_wrapper.py @@ -48,32 +48,38 @@ class EnumTypeWrapper(object): def __init__(self, enum_type): """Inits EnumTypeWrapper with an EnumDescriptor.""" self._enum_type = enum_type - self.DESCRIPTOR = enum_type + self.DESCRIPTOR = enum_type # pylint: disable=invalid-name - def Name(self, number): + def Name(self, number): # pylint: disable=invalid-name """Returns a string containing the name of an enum value.""" - if number in self._enum_type.values_by_number: + try: return self._enum_type.values_by_number[number].name + except KeyError: + pass # fall out to break exception chaining if not isinstance(number, six.integer_types): - raise TypeError('Enum value for %s must be an int, but got %r.' % ( - self._enum_type.name, number)) + raise TypeError( + 'Enum value for {} must be an int, but got {} {!r}.'.format( + self._enum_type.name, type(number), number)) else: - # %r here to handle the odd case when you pass in a boolean. - raise ValueError('Enum %s has no name defined for value %r' % ( + # repr here to handle the odd case when you pass in a boolean. + raise ValueError('Enum {} has no name defined for value {!r}'.format( self._enum_type.name, number)) - def Value(self, name): + def Value(self, name): # pylint: disable=invalid-name """Returns the value corresponding to the given enum name.""" - if name in self._enum_type.values_by_name: + try: return self._enum_type.values_by_name[name].number - raise ValueError('Enum %s has no value defined for name %s' % ( + except KeyError: + pass # fall out to break exception chaining + raise ValueError('Enum {} has no value defined for name {!r}'.format( self._enum_type.name, name)) def keys(self): """Return a list of the string names in the enum. - These are returned in the order they were defined in the .proto file. + Returns: + A list of strs, in the order they were defined in the .proto file. """ return [value_descriptor.name @@ -82,7 +88,8 @@ class EnumTypeWrapper(object): def values(self): """Return a list of the integer values in the enum. - These are returned in the order they were defined in the .proto file. + Returns: + A list of ints, in the order they were defined in the .proto file. """ return [value_descriptor.number @@ -91,13 +98,18 @@ class EnumTypeWrapper(object): def items(self): """Return a list of the (name, value) pairs of the enum. - These are returned in the order they were defined in the .proto file. + Returns: + A list of (str, int) pairs, in the order they were defined + in the .proto file. """ return [(value_descriptor.name, value_descriptor.number) for value_descriptor in self._enum_type.values] def __getattr__(self, name): """Returns the value corresponding to the given enum name.""" - if name in self._enum_type.values_by_name: + try: return self._enum_type.values_by_name[name].number - raise AttributeError + except KeyError: + pass # fall out to break exception chaining + raise AttributeError('Enum {} has no value defined for name {!r}'.format( + self._enum_type.name, name)) diff --git a/python/google/protobuf/internal/json_format_test.py b/python/google/protobuf/internal/json_format_test.py index 2d4267456c..d82827231e 100755 --- a/python/google/protobuf/internal/json_format_test.py +++ b/python/google/protobuf/internal/json_format_test.py @@ -1157,6 +1157,19 @@ class JsonFormatTest(JsonFormatBase): 'Failed to parse any_value field: Can not find message descriptor by' ' type_url: type.googleapis.com/proto3.MessageType..') + def testParseDictUnknownValueType(self): + class UnknownClass(object): + + def __str__(self): + return 'v' + message = json_format_proto3_pb2.TestValue() + self.assertRaisesRegexp( + json_format.ParseError, + r"Value v has unexpected type .", + json_format.ParseDict, + {'value': UnknownClass()}, + message) + def testMessageToDict(self): message = json_format_proto3_pb2.TestMessage() message.int32_value = 12345 diff --git a/python/google/protobuf/json_format.py b/python/google/protobuf/json_format.py index 5b3f8b45cb..70912c1e1b 100644 --- a/python/google/protobuf/json_format.py +++ b/python/google/protobuf/json_format.py @@ -647,7 +647,8 @@ class _Parser(object): elif isinstance(value, _INT_OR_FLOAT): message.number_value = value else: - raise ParseError('Unexpected type for Value message.') + raise ParseError('Value {0} has unexpected type {1}.'.format( + value, type(value))) def _ConvertListValueMessage(self, value, message): """Convert a JSON representation into ListValue message.""" diff --git a/ruby/Rakefile b/ruby/Rakefile index 53241fd515..2aa7743e20 100644 --- a/ruby/Rakefile +++ b/ruby/Rakefile @@ -73,7 +73,7 @@ else ['x86-mingw32', 'x64-mingw32', 'x86_64-linux', 'x86-linux'].each do |plat| RakeCompilerDock.sh <<-"EOT", platform: plat bundle && \ - IN_DOCKER=true rake native:#{plat} pkg/#{spec.full_name}-#{plat}.gem RUBY_CC_VERSION=2.7.0:2.6.0:2.5.0 + IN_DOCKER=true rake native:#{plat} pkg/#{spec.full_name}-#{plat}.gem RUBY_CC_VERSION=2.7.0:2.6.0:2.5.0:2.4.0:2.3.0 EOT end end @@ -81,7 +81,7 @@ else if RUBY_PLATFORM =~ /darwin/ task 'gem:native' do system "rake genproto" - system "rake cross native gem RUBY_CC_VERSION=2.7.0:2.6.0:2.5.1" + system "rake cross native gem RUBY_CC_VERSION=2.7.0:2.6.0:2.5.1:2.4.0:2.3.0" end else task 'gem:native' => [:genproto, 'gem:windows'] diff --git a/ruby/google-protobuf.gemspec b/ruby/google-protobuf.gemspec index 29af94801a..17df6fec50 100644 --- a/ruby/google-protobuf.gemspec +++ b/ruby/google-protobuf.gemspec @@ -1,6 +1,6 @@ Gem::Specification.new do |s| s.name = "google-protobuf" - s.version = "3.12.0.rc.2" + s.version = "3.12.0" git_tag = "v#{s.version.to_s.sub('.rc.', '-rc')}" # Converts X.Y.Z.rc.N to vX.Y.Z-rcN, used for the git tag s.licenses = ["BSD-3-Clause"] s.summary = "Protocol Buffers" diff --git a/src/google/protobuf/compiler/cpp/cpp_enum.cc b/src/google/protobuf/compiler/cpp/cpp_enum.cc index c0a03ad40f..7f9754f67f 100644 --- a/src/google/protobuf/compiler/cpp/cpp_enum.cc +++ b/src/google/protobuf/compiler/cpp/cpp_enum.cc @@ -180,14 +180,17 @@ void EnumGenerator::GenerateDefinition(io::Printer* printer) { if (HasDescriptorMethods(descriptor_->file(), options_)) { format( "inline bool $classname$_Parse(\n" - " const std::string& name, $classname$* value) {\n" + " ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, $classname$* " + "value) " + "{\n" " return ::$proto_ns$::internal::ParseNamedEnum<$classname$>(\n" " $classname$_descriptor(), name, value);\n" "}\n"); } else { format( "bool $classname$_Parse(\n" - " const std::string& name, $classname$* value);\n"); + " ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, $classname$* " + "value);\n"); } } @@ -253,7 +256,8 @@ void EnumGenerator::GenerateSymbolImports(io::Printer* printer) const { " return $classname$_Name(enum_t_value);\n" "}\n"); format( - "static inline bool $nested_name$_Parse(const std::string& name,\n" + "static inline bool " + "$nested_name$_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name,\n" " $resolved_name$* value) {\n" " return $classname$_Parse(name, value);\n" "}\n"); @@ -383,7 +387,9 @@ void EnumGenerator::GenerateMethods(int idx, io::Printer* printer) { CountUniqueValues(descriptor_)); format( "bool $classname$_Parse(\n" - " const std::string& name, $classname$* value) {\n" + " ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, $classname$* " + "value) " + "{\n" " int int_value;\n" " bool success = ::$proto_ns$::internal::LookUpEnumValue(\n" " $classname$_entries, $1$, name, &int_value);\n" diff --git a/src/google/protobuf/compiler/java/java_helpers.cc b/src/google/protobuf/compiler/java/java_helpers.cc index 2b9b039e0c..6c4d894e69 100644 --- a/src/google/protobuf/compiler/java/java_helpers.cc +++ b/src/google/protobuf/compiler/java/java_helpers.cc @@ -75,8 +75,8 @@ const char* kForbiddenWordList[] = { "class", }; -const std::unordered_set* kReservedNames = - new std::unordered_set({ +const std::unordered_set* kReservedNames = + new std::unordered_set({ "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", "class", "const", "continue", "default", "do", "double", "else", diff --git a/src/google/protobuf/compiler/java/java_name_resolver.cc b/src/google/protobuf/compiler/java/java_name_resolver.cc index 361cfa9867..d998f166a3 100644 --- a/src/google/protobuf/compiler/java/java_name_resolver.cc +++ b/src/google/protobuf/compiler/java/java_name_resolver.cc @@ -91,7 +91,7 @@ std::string ClassNameWithoutPackage(const ServiceDescriptor* descriptor, } // Return true if a and b are equals (case insensitive). -NameEquality CheckNameEquality(const string& a, const string& b) { +NameEquality CheckNameEquality(const std::string& a, const std::string& b) { if (ToUpper(a) == ToUpper(b)) { if (a == b) { return NameEquality::EXACT_EQUAL; diff --git a/src/google/protobuf/compiler/plugin.pb.h b/src/google/protobuf/compiler/plugin.pb.h index 2ae03620c5..c6148326fa 100644 --- a/src/google/protobuf/compiler/plugin.pb.h +++ b/src/google/protobuf/compiler/plugin.pb.h @@ -106,7 +106,7 @@ inline const std::string& CodeGeneratorResponse_Feature_Name(T enum_t_value) { CodeGeneratorResponse_Feature_descriptor(), enum_t_value); } inline bool CodeGeneratorResponse_Feature_Parse( - const std::string& name, CodeGeneratorResponse_Feature* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, CodeGeneratorResponse_Feature* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( CodeGeneratorResponse_Feature_descriptor(), name, value); } @@ -924,7 +924,7 @@ class PROTOC_EXPORT CodeGeneratorResponse PROTOBUF_FINAL : "Incorrect type passed to function Feature_Name."); return CodeGeneratorResponse_Feature_Name(enum_t_value); } - static inline bool Feature_Parse(const std::string& name, + static inline bool Feature_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Feature* value) { return CodeGeneratorResponse_Feature_Parse(name, value); } diff --git a/src/google/protobuf/compiler/python/python_generator.cc b/src/google/protobuf/compiler/python/python_generator.cc index 050fe9edef..88804bd30b 100644 --- a/src/google/protobuf/compiler/python/python_generator.cc +++ b/src/google/protobuf/compiler/python/python_generator.cc @@ -125,7 +125,7 @@ bool ContainsPythonKeyword(const std::string& module_name) { return false; } -inline bool IsPythonKeyword(const string& name) { +inline bool IsPythonKeyword(const std::string& name) { return (std::find(kKeywords, kKeywordsEnd, name) != kKeywordsEnd); } diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index aaaec6ab67..5001222b82 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -383,19 +383,8 @@ class PrefixRemover { // hash-maps for each object. // // The keys to these hash-maps are (parent, name) or (parent, number) pairs. -// -// TODO(kenton): Use StringPiece rather than const char* in keys? It would -// be a lot cleaner but we'd just have to convert it back to const char* -// for the open source release. -typedef std::pair PointerStringPair; - -struct PointerStringPairEqual { - inline bool operator()(const PointerStringPair& a, - const PointerStringPair& b) const { - return a.first == b.first && strcmp(a.second, b.second) == 0; - } -}; +typedef std::pair PointerStringPair; typedef std::pair DescriptorIntPair; typedef std::pair EnumIntPair; @@ -419,16 +408,16 @@ struct PointerIntegerPairHash { static const size_t min_buckets = 8; #endif inline bool operator()(const PairType& a, const PairType& b) const { - return a.first < b.first || (a.first == b.first && a.second < b.second); + return a < b; } }; struct PointerStringPairHash { size_t operator()(const PointerStringPair& p) const { static const size_t prime = 16777619; - hash cstring_hash; + hash string_hash; return reinterpret_cast(p.first) * prime ^ - static_cast(cstring_hash(p.second)); + static_cast(string_hash(p.second)); } #ifdef _MSC_VER @@ -438,9 +427,7 @@ struct PointerStringPairHash { #endif inline bool operator()(const PointerStringPair& a, const PointerStringPair& b) const { - if (a.first < b.first) return true; - if (a.first > b.first) return false; - return strcmp(a.second, b.second) < 0; + return a < b; } }; @@ -450,8 +437,7 @@ const Symbol kNullSymbol; typedef HASH_MAP, streq> SymbolsByNameMap; -typedef HASH_MAP +typedef HASH_MAP SymbolsByParentMap; typedef HASH_MAP, @@ -459,7 +445,7 @@ typedef HASH_MAP, FilesByNameMap; typedef HASH_MAP + PointerStringPairHash> FieldsByNameMap; typedef HASH_MAPtables_->FindNestedSymbolOfType(this, key, Symbol::ENUM_VALUE); if (!result.IsNull()) { diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h index b55841f97a..f0d2064920 100644 --- a/src/google/protobuf/descriptor.h +++ b/src/google/protobuf/descriptor.h @@ -63,6 +63,7 @@ #include #include #include +#include #include // TYPE_BOOL is defined in the MacOS's ConditionalMacros.h. @@ -1029,7 +1030,7 @@ class PROTOBUF_EXPORT EnumDescriptor { const EnumValueDescriptor* value(int index) const; // Looks up a value by name. Returns nullptr if no such value exists. - const EnumValueDescriptor* FindValueByName(const std::string& name) const; + const EnumValueDescriptor* FindValueByName(ConstStringParam name) const; // Looks up a value by number. Returns nullptr if no such value exists. If // multiple values have this number, the first one defined is returned. const EnumValueDescriptor* FindValueByNumber(int number) const; diff --git a/src/google/protobuf/descriptor.pb.h b/src/google/protobuf/descriptor.pb.h index f8fc5544a4..b41f228fb6 100644 --- a/src/google/protobuf/descriptor.pb.h +++ b/src/google/protobuf/descriptor.pb.h @@ -204,7 +204,7 @@ inline const std::string& FieldDescriptorProto_Type_Name(T enum_t_value) { FieldDescriptorProto_Type_descriptor(), enum_t_value); } inline bool FieldDescriptorProto_Type_Parse( - const std::string& name, FieldDescriptorProto_Type* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, FieldDescriptorProto_Type* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( FieldDescriptorProto_Type_descriptor(), name, value); } @@ -228,7 +228,7 @@ inline const std::string& FieldDescriptorProto_Label_Name(T enum_t_value) { FieldDescriptorProto_Label_descriptor(), enum_t_value); } inline bool FieldDescriptorProto_Label_Parse( - const std::string& name, FieldDescriptorProto_Label* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, FieldDescriptorProto_Label* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( FieldDescriptorProto_Label_descriptor(), name, value); } @@ -252,7 +252,7 @@ inline const std::string& FileOptions_OptimizeMode_Name(T enum_t_value) { FileOptions_OptimizeMode_descriptor(), enum_t_value); } inline bool FileOptions_OptimizeMode_Parse( - const std::string& name, FileOptions_OptimizeMode* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, FileOptions_OptimizeMode* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( FileOptions_OptimizeMode_descriptor(), name, value); } @@ -276,7 +276,7 @@ inline const std::string& FieldOptions_CType_Name(T enum_t_value) { FieldOptions_CType_descriptor(), enum_t_value); } inline bool FieldOptions_CType_Parse( - const std::string& name, FieldOptions_CType* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, FieldOptions_CType* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( FieldOptions_CType_descriptor(), name, value); } @@ -300,7 +300,7 @@ inline const std::string& FieldOptions_JSType_Name(T enum_t_value) { FieldOptions_JSType_descriptor(), enum_t_value); } inline bool FieldOptions_JSType_Parse( - const std::string& name, FieldOptions_JSType* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, FieldOptions_JSType* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( FieldOptions_JSType_descriptor(), name, value); } @@ -324,7 +324,7 @@ inline const std::string& MethodOptions_IdempotencyLevel_Name(T enum_t_value) { MethodOptions_IdempotencyLevel_descriptor(), enum_t_value); } inline bool MethodOptions_IdempotencyLevel_Parse( - const std::string& name, MethodOptions_IdempotencyLevel* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, MethodOptions_IdempotencyLevel* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( MethodOptions_IdempotencyLevel_descriptor(), name, value); } @@ -1936,7 +1936,7 @@ class PROTOBUF_EXPORT FieldDescriptorProto PROTOBUF_FINAL : "Incorrect type passed to function Type_Name."); return FieldDescriptorProto_Type_Name(enum_t_value); } - static inline bool Type_Parse(const std::string& name, + static inline bool Type_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Type* value) { return FieldDescriptorProto_Type_Parse(name, value); } @@ -1968,7 +1968,7 @@ class PROTOBUF_EXPORT FieldDescriptorProto PROTOBUF_FINAL : "Incorrect type passed to function Label_Name."); return FieldDescriptorProto_Label_Name(enum_t_value); } - static inline bool Label_Parse(const std::string& name, + static inline bool Label_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Label* value) { return FieldDescriptorProto_Label_Parse(name, value); } @@ -3668,7 +3668,7 @@ class PROTOBUF_EXPORT FileOptions PROTOBUF_FINAL : "Incorrect type passed to function OptimizeMode_Name."); return FileOptions_OptimizeMode_Name(enum_t_value); } - static inline bool OptimizeMode_Parse(const std::string& name, + static inline bool OptimizeMode_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, OptimizeMode* value) { return FileOptions_OptimizeMode_Parse(name, value); } @@ -4534,7 +4534,7 @@ class PROTOBUF_EXPORT FieldOptions PROTOBUF_FINAL : "Incorrect type passed to function CType_Name."); return FieldOptions_CType_Name(enum_t_value); } - static inline bool CType_Parse(const std::string& name, + static inline bool CType_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, CType* value) { return FieldOptions_CType_Parse(name, value); } @@ -4566,7 +4566,7 @@ class PROTOBUF_EXPORT FieldOptions PROTOBUF_FINAL : "Incorrect type passed to function JSType_Name."); return FieldOptions_JSType_Name(enum_t_value); } - static inline bool JSType_Parse(const std::string& name, + static inline bool JSType_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, JSType* value) { return FieldOptions_JSType_Parse(name, value); } @@ -5532,7 +5532,7 @@ class PROTOBUF_EXPORT MethodOptions PROTOBUF_FINAL : "Incorrect type passed to function IdempotencyLevel_Name."); return MethodOptions_IdempotencyLevel_Name(enum_t_value); } - static inline bool IdempotencyLevel_Parse(const std::string& name, + static inline bool IdempotencyLevel_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, IdempotencyLevel* value) { return MethodOptions_IdempotencyLevel_Parse(name, value); } diff --git a/src/google/protobuf/descriptor_database.cc b/src/google/protobuf/descriptor_database.cc index ae25a4681f..dbe1dc1740 100644 --- a/src/google/protobuf/descriptor_database.cc +++ b/src/google/protobuf/descriptor_database.cc @@ -41,6 +41,7 @@ #include #include + namespace google { namespace protobuf { @@ -389,11 +390,14 @@ bool SimpleDescriptorDatabase::MaybeCopy(const FileDescriptorProto* file, // ------------------------------------------------------------------- class EncodedDescriptorDatabase::DescriptorIndex { + using String = std::string; + public: using Value = std::pair; // Helpers to recursively add particular descriptors and all their contents // to the index. - bool AddFile(const FileDescriptorProto& file, Value value); + template + bool AddFile(const FileProto& file, Value value); Value FindFile(StringPiece filename); Value FindSymbol(StringPiece name); @@ -406,11 +410,15 @@ class EncodedDescriptorDatabase::DescriptorIndex { private: friend class EncodedDescriptorDatabase; - bool AddSymbol(StringPiece name, Value value); + bool AddSymbol(StringPiece package, StringPiece symbol, + Value value); + + template bool AddNestedExtensions(StringPiece filename, - const DescriptorProto& message_type, Value value); - bool AddExtension(StringPiece filename, - const FieldDescriptorProto& field, Value value); + const DescProto& message_type, Value value); + template + bool AddExtension(StringPiece filename, const FieldProto& field, + Value value); // All the maps below have two representations: // - a std::set<> where we insert initially. @@ -421,27 +429,72 @@ class EncodedDescriptorDatabase::DescriptorIndex { void EnsureFlat(); - struct Entry { - std::string name; + struct FileEntry { + String name; Value data; }; - struct Compare { - bool operator()(const Entry& a, const Entry& b) const { + struct FileCompare { + bool operator()(const FileEntry& a, const FileEntry& b) const { return a.name < b.name; } - bool operator()(const Entry& a, StringPiece b) const { + bool operator()(const FileEntry& a, StringPiece b) const { return a.name < b; } - bool operator()(StringPiece a, const Entry& b) const { + bool operator()(StringPiece a, const FileEntry& b) const { return a < b.name; } }; - std::set by_name_; - std::vector by_name_flat_; - std::set by_symbol_; - std::vector by_symbol_flat_; + std::set by_name_; + std::vector by_name_flat_; + + struct SymbolEntry { + String package; + String symbol; + Value data; + + std::string AsString() const { + return StrCat(package, package.empty() ? "" : ".", symbol); + } + }; + + struct SymbolCompare { + static std::string AsString(const SymbolEntry& entry) { + return entry.AsString(); + } + static StringPiece AsString(StringPiece str) { return str; } + + static std::pair GetParts( + const SymbolEntry& entry) { + if (entry.package.empty()) return {entry.symbol, StringPiece{}}; + return {entry.package, entry.symbol}; + } + static std::pair GetParts( + StringPiece str) { + return {str, {}}; + } + + template + bool operator()(const T& lhs, const U& rhs) const { + auto lhs_parts = GetParts(lhs); + auto rhs_parts = GetParts(rhs); + + // Fast path to avoid making the whole string for common cases. + if (int res = + lhs_parts.first.substr(0, rhs_parts.first.size()) + .compare(rhs_parts.first.substr(0, lhs_parts.first.size()))) { + // If the packages already differ, exit early. + return res < 0; + } else if (lhs_parts.first.size() == rhs_parts.first.size()) { + return lhs_parts.second < rhs_parts.second; + } + return AsString(lhs) < AsString(rhs); + } + }; + std::set by_symbol_; + std::vector by_symbol_flat_; + struct ExtensionEntry { - std::string extendee; + String extendee; int extension_number; Value data; }; @@ -535,34 +588,30 @@ bool EncodedDescriptorDatabase::FindAllExtensionNumbers( return index_->FindAllExtensionNumbers(extendee_type, output); } -bool EncodedDescriptorDatabase::DescriptorIndex::AddFile( - const FileDescriptorProto& file, Value value) { - if (!InsertIfNotPresent(&by_name_, Entry{file.name(), value}) || +template +bool EncodedDescriptorDatabase::DescriptorIndex::AddFile(const FileProto& file, + Value value) { + if (!InsertIfNotPresent(&by_name_, FileEntry{file.name(), value}) || std::binary_search(by_name_flat_.begin(), by_name_flat_.end(), file.name(), by_name_.key_comp())) { GOOGLE_LOG(ERROR) << "File already exists in database: " << file.name(); return false; } - // We must be careful here -- calling file.package() if file.has_package() is - // false could access an uninitialized static-storage variable if we are being - // run at startup time. - std::string path = file.has_package() ? file.package() : std::string(); - if (!path.empty()) path += '.'; - + StringPiece package = file.package(); for (const auto& message_type : file.message_type()) { - if (!AddSymbol(path + message_type.name(), value)) return false; + if (!AddSymbol(package, message_type.name(), value)) return false; if (!AddNestedExtensions(file.name(), message_type, value)) return false; } for (const auto& enum_type : file.enum_type()) { - if (!AddSymbol(path + enum_type.name(), value)) return false; + if (!AddSymbol(package, enum_type.name(), value)) return false; } for (const auto& extension : file.extension()) { - if (!AddSymbol(path + extension.name(), value)) return false; + if (!AddSymbol(package, extension.name(), value)) return false; if (!AddExtension(file.name(), extension, value)) return false; } for (const auto& service : file.service()) { - if (!AddSymbol(path + service.name(), value)) return false; + if (!AddSymbol(package, service.name(), value)) return false; } return true; @@ -572,10 +621,10 @@ template static bool CheckForMutualSubsymbols(StringPiece symbol_name, Iter* iter, Iter2 end) { if (*iter != end) { - if (IsSubSymbol((*iter)->name, symbol_name)) { + if (IsSubSymbol((*iter)->AsString(), symbol_name)) { GOOGLE_LOG(ERROR) << "Symbol name \"" << symbol_name - << "\" conflicts with the existing symbol \"" << (*iter)->name - << "\"."; + << "\" conflicts with the existing symbol \"" + << (*iter)->AsString() << "\"."; return false; } @@ -586,10 +635,10 @@ static bool CheckForMutualSubsymbols(StringPiece symbol_name, Iter* iter, // to increment it. ++*iter; - if (*iter != end && IsSubSymbol(symbol_name, (*iter)->name)) { + if (*iter != end && IsSubSymbol(symbol_name, (*iter)->AsString())) { GOOGLE_LOG(ERROR) << "Symbol name \"" << symbol_name - << "\" conflicts with the existing symbol \"" << (*iter)->name - << "\"."; + << "\" conflicts with the existing symbol \"" + << (*iter)->AsString() << "\"."; return false; } } @@ -597,28 +646,30 @@ static bool CheckForMutualSubsymbols(StringPiece symbol_name, Iter* iter, } bool EncodedDescriptorDatabase::DescriptorIndex::AddSymbol( - StringPiece name, Value value) { + StringPiece package, StringPiece symbol, Value value) { + SymbolEntry entry = {String(package), String(symbol), value}; + std::string entry_as_string = entry.AsString(); + // We need to make sure not to violate our map invariant. // If the symbol name is invalid it could break our lookup algorithm (which // relies on the fact that '.' sorts before all other characters that are // valid in symbol names). - if (!ValidateSymbolName(name)) { - GOOGLE_LOG(ERROR) << "Invalid symbol name: " << name; + if (!ValidateSymbolName(package) || !ValidateSymbolName(symbol)) { + GOOGLE_LOG(ERROR) << "Invalid symbol name: " << entry_as_string; return false; } - Entry entry = {std::string(name), value}; - auto iter = FindLastLessOrEqual(&by_symbol_, entry); - if (!CheckForMutualSubsymbols(name, &iter, by_symbol_.end())) { + if (!CheckForMutualSubsymbols(entry_as_string, &iter, by_symbol_.end())) { return false; } // Same, but on by_symbol_flat_ auto flat_iter = - FindLastLessOrEqual(&by_symbol_flat_, name, by_symbol_.key_comp()); - if (!CheckForMutualSubsymbols(name, &flat_iter, by_symbol_flat_.end())) { + FindLastLessOrEqual(&by_symbol_flat_, entry, by_symbol_.key_comp()); + if (!CheckForMutualSubsymbols(entry_as_string, &flat_iter, + by_symbol_flat_.end())) { return false; } @@ -626,14 +677,14 @@ bool EncodedDescriptorDatabase::DescriptorIndex::AddSymbol( // Insert the new symbol using the iterator as a hint, the new entry will // appear immediately before the one the iterator is pointing at. - by_symbol_.insert(iter, std::move(entry)); + by_symbol_.insert(iter, entry); return true; } +template bool EncodedDescriptorDatabase::DescriptorIndex::AddNestedExtensions( - StringPiece filename, const DescriptorProto& message_type, - Value value) { + StringPiece filename, const DescProto& message_type, Value value) { for (const auto& nested_type : message_type.nested_type()) { if (!AddNestedExtensions(filename, nested_type, value)) return false; } @@ -643,9 +694,9 @@ bool EncodedDescriptorDatabase::DescriptorIndex::AddNestedExtensions( return true; } +template bool EncodedDescriptorDatabase::DescriptorIndex::AddExtension( - StringPiece filename, const FieldDescriptorProto& field, - Value value) { + StringPiece filename, const FieldProto& field, Value value) { if (!field.extendee().empty() && field.extendee()[0] == '.') { // The extension is fully-qualified. We can use it as a lookup key in // the by_symbol_ table. @@ -682,7 +733,7 @@ EncodedDescriptorDatabase::DescriptorIndex::FindSymbolOnlyFlat( auto iter = FindLastLessOrEqual(&by_symbol_flat_, name, by_symbol_.key_comp()); - return iter != by_symbol_flat_.end() && IsSubSymbol(iter->name, name) + return iter != by_symbol_flat_.end() && IsSubSymbol(iter->AsString(), name) ? iter->data : Value(); } @@ -740,11 +791,11 @@ void EncodedDescriptorDatabase::DescriptorIndex::FindAllFileNames( output->resize(by_name_.size() + by_name_flat_.size()); int i = 0; for (const auto& entry : by_name_) { - (*output)[i] = entry.name; + (*output)[i] = std::string(entry.name); i++; } for (const auto& entry : by_name_flat_) { - (*output)[i] = entry.name; + (*output)[i] = std::string(entry.name); i++; } } diff --git a/src/google/protobuf/descriptor_database_unittest.cc b/src/google/protobuf/descriptor_database_unittest.cc index 68152835b1..fba0b95a84 100644 --- a/src/google/protobuf/descriptor_database_unittest.cc +++ b/src/google/protobuf/descriptor_database_unittest.cc @@ -547,7 +547,7 @@ TEST(SimpleDescriptorDatabaseExtraTest, FindAllPackageNames) { db.Add(f); db.Add(b); - std::vector packages; + std::vector packages; EXPECT_TRUE(db.FindAllPackageNames(&packages)); EXPECT_THAT(packages, ::testing::UnorderedElementsAre("foo", "")); } @@ -567,7 +567,7 @@ TEST(SimpleDescriptorDatabaseExtraTest, FindAllMessageNames) { db.Add(f); db.Add(b); - std::vector messages; + std::vector messages; EXPECT_TRUE(db.FindAllMessageNames(&messages)); EXPECT_THAT(messages, ::testing::UnorderedElementsAre("foo.Foo", "Bar")); } diff --git a/src/google/protobuf/dynamic_message.cc b/src/google/protobuf/dynamic_message.cc index fd5237172f..2af5109701 100644 --- a/src/google/protobuf/dynamic_message.cc +++ b/src/google/protobuf/dynamic_message.cc @@ -336,26 +336,24 @@ class DynamicMessage : public Message { } const TypeInfo* type_info_; - Arena* const arena_; mutable std::atomic cached_byte_size_; GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(DynamicMessage); }; DynamicMessage::DynamicMessage(const TypeInfo* type_info) - : type_info_(type_info), arena_(NULL), cached_byte_size_(0) { + : type_info_(type_info), cached_byte_size_(0) { SharedCtor(true); } DynamicMessage::DynamicMessage(const TypeInfo* type_info, Arena* arena) : Message(arena), type_info_(type_info), - arena_(arena), cached_byte_size_(0) { SharedCtor(true); } DynamicMessage::DynamicMessage(TypeInfo* type_info, bool lock_factory) - : type_info_(type_info), arena_(NULL), cached_byte_size_(0) { + : type_info_(type_info), cached_byte_size_(0) { // The prototype in type_info has to be set before creating the prototype // instance on memory. e.g., message Foo { map a = 1; }. When // creating prototype for Foo, prototype of the map entry will also be @@ -386,7 +384,8 @@ void DynamicMessage::SharedCtor(bool lock_factory) { } if (type_info_->extensions_offset != -1) { - new (OffsetToPointer(type_info_->extensions_offset)) ExtensionSet(arena_); + new (OffsetToPointer(type_info_->extensions_offset)) + ExtensionSet(GetArena()); } for (int i = 0; i < descriptor->field_count(); i++) { const FieldDescriptor* field = descriptor->field(i); @@ -400,7 +399,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { if (!field->is_repeated()) { \ new (field_ptr) TYPE(field->default_value_##TYPE()); \ } else { \ - new (field_ptr) RepeatedField(arena_); \ + new (field_ptr) RepeatedField(GetArena()); \ } \ break; @@ -417,7 +416,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { if (!field->is_repeated()) { new (field_ptr) int(field->default_value_enum()->number()); } else { - new (field_ptr) RepeatedField(arena_); + new (field_ptr) RepeatedField(GetArena()); } break; @@ -438,7 +437,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { ArenaStringPtr* asp = new (field_ptr) ArenaStringPtr(); asp->UnsafeSetDefault(default_value); } else { - new (field_ptr) RepeatedPtrField(arena_); + new (field_ptr) RepeatedPtrField(GetArena()); } break; } @@ -453,20 +452,20 @@ void DynamicMessage::SharedCtor(bool lock_factory) { // when the constructor is called inside GetPrototype(), in which // case we have already locked the factory. if (lock_factory) { - if (arena_ != NULL) { + if (GetArena() != nullptr) { new (field_ptr) DynamicMapField( type_info_->factory->GetPrototype(field->message_type()), - arena_); + GetArena()); } else { new (field_ptr) DynamicMapField( type_info_->factory->GetPrototype(field->message_type())); } } else { - if (arena_ != NULL) { + if (GetArena() != nullptr) { new (field_ptr) DynamicMapField(type_info_->factory->GetPrototypeNoLock( field->message_type()), - arena_); + GetArena()); } else { new (field_ptr) DynamicMapField(type_info_->factory->GetPrototypeNoLock( @@ -474,7 +473,7 @@ void DynamicMessage::SharedCtor(bool lock_factory) { } } } else { - new (field_ptr) RepeatedPtrField(arena_); + new (field_ptr) RepeatedPtrField(GetArena()); } } break; diff --git a/src/google/protobuf/dynamic_message_unittest.cc b/src/google/protobuf/dynamic_message_unittest.cc index 6db6f5cfaa..37f9574b0b 100644 --- a/src/google/protobuf/dynamic_message_unittest.cc +++ b/src/google/protobuf/dynamic_message_unittest.cc @@ -252,7 +252,7 @@ TEST_P(DynamicMessageTest, Oneof) { } TEST_P(DynamicMessageTest, SpaceUsed) { - // Test that SpaceUsed() works properly + // Test that SpaceUsedLong() works properly // Since we share the implementation with generated messages, we don't need // to test very much here. Just make sure it appears to be working. @@ -261,10 +261,10 @@ TEST_P(DynamicMessageTest, SpaceUsed) { Message* message = prototype_->New(GetParam() ? &arena : NULL); TestUtil::ReflectionTester reflection_tester(descriptor_); - int initial_space_used = message->SpaceUsed(); + size_t initial_space_used = message->SpaceUsedLong(); reflection_tester.SetAllFieldsViaReflection(message); - EXPECT_LT(initial_space_used, message->SpaceUsed()); + EXPECT_LT(initial_space_used, message->SpaceUsedLong()); if (!GetParam()) { delete message; diff --git a/src/google/protobuf/extension_set_unittest.cc b/src/google/protobuf/extension_set_unittest.cc index ccda9930ec..d5073eeb17 100644 --- a/src/google/protobuf/extension_set_unittest.cc +++ b/src/google/protobuf/extension_set_unittest.cc @@ -514,7 +514,7 @@ TEST(ExtensionSetTest, SerializationToArray) { unittest::TestAllExtensions source; unittest::TestAllTypes destination; TestUtil::SetAllExtensions(&source); - int size = source.ByteSize(); + size_t size = source.ByteSizeLong(); std::string data; data.resize(size); uint8* target = reinterpret_cast(::google::protobuf::string_as_array(&data)); @@ -535,7 +535,7 @@ TEST(ExtensionSetTest, SerializationToStream) { unittest::TestAllExtensions source; unittest::TestAllTypes destination; TestUtil::SetAllExtensions(&source); - int size = source.ByteSize(); + size_t size = source.ByteSizeLong(); std::string data; data.resize(size); { @@ -558,7 +558,7 @@ TEST(ExtensionSetTest, PackedSerializationToArray) { unittest::TestPackedExtensions source; unittest::TestPackedTypes destination; TestUtil::SetPackedExtensions(&source); - int size = source.ByteSize(); + size_t size = source.ByteSizeLong(); std::string data; data.resize(size); uint8* target = reinterpret_cast(::google::protobuf::string_as_array(&data)); @@ -579,7 +579,7 @@ TEST(ExtensionSetTest, PackedSerializationToStream) { unittest::TestPackedExtensions source; unittest::TestPackedTypes destination; TestUtil::SetPackedExtensions(&source); - int size = source.ByteSize(); + size_t size = source.ByteSizeLong(); std::string data; data.resize(size); { @@ -739,12 +739,12 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { #define TEST_SCALAR_EXTENSIONS_SPACE_USED(type, value) \ do { \ unittest::TestAllExtensions message; \ - const int base_size = message.SpaceUsed(); \ + const int base_size = message.SpaceUsedLong(); \ message.SetExtension(unittest::optional_##type##_extension, value); \ int min_expected_size = \ base_size + \ sizeof(message.GetExtension(unittest::optional_##type##_extension)); \ - EXPECT_LE(min_expected_size, message.SpaceUsed()); \ + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); \ } while (0) TEST_SCALAR_EXTENSIONS_SPACE_USED(int32, 101); @@ -763,36 +763,36 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { #undef TEST_SCALAR_EXTENSIONS_SPACE_USED { unittest::TestAllExtensions message; - const int base_size = message.SpaceUsed(); + const int base_size = message.SpaceUsedLong(); message.SetExtension(unittest::optional_nested_enum_extension, unittest::TestAllTypes::FOO); int min_expected_size = base_size + sizeof(message.GetExtension(unittest::optional_nested_enum_extension)); - EXPECT_LE(min_expected_size, message.SpaceUsed()); + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); } { // Strings may cause extra allocations depending on their length; ensure // that gets included as well. unittest::TestAllExtensions message; - const int base_size = message.SpaceUsed(); + const int base_size = message.SpaceUsedLong(); const std::string s( "this is a fairly large string that will cause some " "allocation in order to store it in the extension"); message.SetExtension(unittest::optional_string_extension, s); int min_expected_size = base_size + s.length(); - EXPECT_LE(min_expected_size, message.SpaceUsed()); + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); } { // Messages also have additional allocation that need to be counted. unittest::TestAllExtensions message; - const int base_size = message.SpaceUsed(); + const int base_size = message.SpaceUsedLong(); unittest::ForeignMessage foreign; foreign.set_c(42); message.MutableExtension(unittest::optional_foreign_message_extension) ->CopyFrom(foreign); - int min_expected_size = base_size + foreign.SpaceUsed(); - EXPECT_LE(min_expected_size, message.SpaceUsed()); + int min_expected_size = base_size + foreign.SpaceUsedLong(); + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); } // Repeated primitive extensions will increase space used by at least a @@ -802,23 +802,23 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { // - Adds a value to the repeated extension, then clears it, establishing // the base size. // - Adds a small number of values, testing that it doesn't increase the - // SpaceUsed() + // SpaceUsedLong() // - Adds a large number of values (requiring allocation in the repeated - // field), and ensures that that allocation is included in SpaceUsed() + // field), and ensures that that allocation is included in SpaceUsedLong() #define TEST_REPEATED_EXTENSIONS_SPACE_USED(type, cpptype, value) \ do { \ unittest::TestAllExtensions message; \ - const int base_size = message.SpaceUsed(); \ - int min_expected_size = sizeof(RepeatedField) + base_size; \ + const size_t base_size = message.SpaceUsedLong(); \ + size_t min_expected_size = sizeof(RepeatedField) + base_size; \ message.AddExtension(unittest::repeated_##type##_extension, value); \ message.ClearExtension(unittest::repeated_##type##_extension); \ - const int empty_repeated_field_size = message.SpaceUsed(); \ + const size_t empty_repeated_field_size = message.SpaceUsedLong(); \ EXPECT_LE(min_expected_size, empty_repeated_field_size) << #type; \ message.AddExtension(unittest::repeated_##type##_extension, value); \ message.AddExtension(unittest::repeated_##type##_extension, value); \ - EXPECT_EQ(empty_repeated_field_size, message.SpaceUsed()) << #type; \ + EXPECT_EQ(empty_repeated_field_size, message.SpaceUsedLong()) << #type; \ message.ClearExtension(unittest::repeated_##type##_extension); \ - const int old_capacity = \ + const size_t old_capacity = \ message.GetRepeatedExtension(unittest::repeated_##type##_extension) \ .Capacity(); \ EXPECT_GE(old_capacity, kRepeatedFieldLowerClampLimit); \ @@ -832,7 +832,7 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { .Capacity() - \ old_capacity) + \ empty_repeated_field_size; \ - EXPECT_LE(expected_size, message.SpaceUsed()) << #type; \ + EXPECT_LE(expected_size, message.SpaceUsedLong()) << #type; \ } while (0) TEST_REPEATED_EXTENSIONS_SPACE_USED(int32, int32, 101); @@ -854,8 +854,9 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { // Repeated strings { unittest::TestAllExtensions message; - const int base_size = message.SpaceUsed(); - int min_expected_size = sizeof(RepeatedPtrField) + base_size; + const size_t base_size = message.SpaceUsedLong(); + size_t min_expected_size = + sizeof(RepeatedPtrField) + base_size; const std::string value(256, 'x'); // Once items are allocated, they may stick around even when cleared so // without the hardcore memory management accessors there isn't a notion of @@ -865,13 +866,13 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { } min_expected_size += (sizeof(value) + value.size()) * (16 - kRepeatedFieldLowerClampLimit); - EXPECT_LE(min_expected_size, message.SpaceUsed()); + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); } // Repeated messages { unittest::TestAllExtensions message; - const int base_size = message.SpaceUsed(); - int min_expected_size = + const size_t base_size = message.SpaceUsedLong(); + size_t min_expected_size = sizeof(RepeatedPtrField) + base_size; unittest::ForeignMessage prototype; prototype.set_c(2); @@ -880,8 +881,8 @@ TEST(ExtensionSetTest, SpaceUsedExcludingSelf) { ->CopyFrom(prototype); } min_expected_size += - (16 - kRepeatedFieldLowerClampLimit) * prototype.SpaceUsed(); - EXPECT_LE(min_expected_size, message.SpaceUsed()); + (16 - kRepeatedFieldLowerClampLimit) * prototype.SpaceUsedLong(); + EXPECT_LE(min_expected_size, message.SpaceUsedLong()); } } diff --git a/src/google/protobuf/generated_enum_reflection.h b/src/google/protobuf/generated_enum_reflection.h index c25e51bc12..989b5a8905 100644 --- a/src/google/protobuf/generated_enum_reflection.h +++ b/src/google/protobuf/generated_enum_reflection.h @@ -43,6 +43,7 @@ #include #include +#include #ifdef SWIG #error "You cannot SWIG proto headers" @@ -71,10 +72,10 @@ namespace internal { // an enum name of the given type, returning true and filling in value on // success, or returning false and leaving value unchanged on failure. PROTOBUF_EXPORT bool ParseNamedEnum(const EnumDescriptor* descriptor, - const std::string& name, int* value); + StringPiece name, int* value); template -bool ParseNamedEnum(const EnumDescriptor* descriptor, const std::string& name, +bool ParseNamedEnum(const EnumDescriptor* descriptor, StringPiece name, EnumType* value) { int tmp; if (!ParseNamedEnum(descriptor, name, &tmp)) return false; diff --git a/src/google/protobuf/generated_message_reflection.cc b/src/google/protobuf/generated_message_reflection.cc index 85b51e39a3..be1e740726 100644 --- a/src/google/protobuf/generated_message_reflection.cc +++ b/src/google/protobuf/generated_message_reflection.cc @@ -50,6 +50,7 @@ #include #include #include +#include #include @@ -81,7 +82,7 @@ bool IsMapFieldInApi(const FieldDescriptor* field) { return field->is_map(); } namespace internal { -bool ParseNamedEnum(const EnumDescriptor* descriptor, const std::string& name, +bool ParseNamedEnum(const EnumDescriptor* descriptor, StringPiece name, int* value) { const EnumValueDescriptor* d = descriptor->FindValueByName(name); if (d == nullptr) return false; diff --git a/src/google/protobuf/lite_unittest.cc b/src/google/protobuf/lite_unittest.cc index cd5ccc4eab..ea18a20883 100644 --- a/src/google/protobuf/lite_unittest.cc +++ b/src/google/protobuf/lite_unittest.cc @@ -613,7 +613,7 @@ TEST(Lite, AllLite28) { protobuf_unittest::TestMapLite message1, message2; std::string data; MapLiteTestUtil::SetMapFields(&message1); - int size = message1.ByteSize(); + size_t size = message1.ByteSizeLong(); data.resize(size); ::google::protobuf::uint8* start = reinterpret_cast<::google::protobuf::uint8*>(::google::protobuf::string_as_array(&data)); ::google::protobuf::uint8* end = message1.SerializeWithCachedSizesToArray(start); @@ -630,7 +630,7 @@ TEST(Lite, AllLite29) { // Test the generated SerializeWithCachedSizes() protobuf_unittest::TestMapLite message1, message2; MapLiteTestUtil::SetMapFields(&message1); - int size = message1.ByteSize(); + size_t size = message1.ByteSizeLong(); std::string data; data.resize(size); { diff --git a/src/google/protobuf/map.h b/src/google/protobuf/map.h index bf0e465d8e..786fcf9de0 100644 --- a/src/google/protobuf/map.h +++ b/src/google/protobuf/map.h @@ -42,9 +42,14 @@ #include #include // To support Visual Studio 2008 #include +#include #include #include +#if defined(__cpp_lib_string_view) +#include +#endif // defined(__cpp_lib_string_view) + #include #include #include @@ -186,6 +191,67 @@ using KeyForTree = typename std::conditional::value, T, std::reference_wrapper>::type; +// Default case: Not transparent. +// We use std::hash/std::less and all the lookup functions +// only accept `key_type`. +template +struct TransparentSupport { + using hash = std::hash; + using less = std::less; + + static bool Equals(const key_type& a, const key_type& b) { return a == b; } + + template + using key_arg = key_type; +}; + +#if defined(__cpp_lib_string_view) +// If std::string_view is available, we add transparent support for std::string +// keys. We use std::hash as it supports the input types we +// care about. The lookup functions accept arbitrary `K`. This will include any +// key type that is convertible to std::string_view. +template <> +struct TransparentSupport { + static std::string_view ImplicitConvert(std::string_view str) { return str; } + // If the element is not convertible to std::string_view, try to convert to + // std::string first. + // The template makes this overload lose resolution when both have the same + // rank otherwise. + template + static std::string_view ImplicitConvert(const std::string& str) { + return str; + } + + struct hash : private std::hash { + using is_transparent = void; + + template + size_t operator()(const T& str) const { + return base()(ImplicitConvert(str)); + } + + private: + const std::hash& base() const { return *this; } + }; + struct less { + using is_transparent = void; + + template + bool operator()(const T& t, const U& u) const { + return ImplicitConvert(t) < ImplicitConvert(u); + } + }; + + template + static bool Equals(const T& t, const U& u) { + return ImplicitConvert(t) == ImplicitConvert(u); + } + + template + using key_arg = K; +}; +#endif // defined(__cpp_lib_string_view) + } // namespace internal // This is the class for Map's internal value_type. Instead of using @@ -240,7 +306,7 @@ class Map { using const_reference = const value_type&; using size_type = size_t; - using hasher = std::hash; + using hasher = typename internal::TransparentSupport::hash; Map() : arena_(nullptr), default_enum_value_(0) { Init(); } explicit Map(Arena* arena) : arena_(arena), default_enum_value_(0) { Init(); } @@ -357,7 +423,8 @@ class Map { // class per key type. using TreeAllocator = typename Allocator::template rebind< std::pair, void*>>::other; - using Tree = std::map, void*, std::less, + using Tree = std::map, void*, + typename internal::TransparentSupport::less, TreeAllocator>; using TreeIterator = typename Tree::iterator; @@ -541,9 +608,10 @@ class Map { size_type size() const { return num_elements_; } bool empty() const { return size() == 0; } - iterator find(const Key& k) { return iterator(FindHelper(k).first); } - const_iterator find(const Key& k) const { return find(k, nullptr); } - bool contains(const Key& k) const { return find(k) != end(); } + template + iterator find(const K& k) { + return iterator(FindHelper(k).first); + } // Insert the key into the map, if not present. In that case, the value will // be value initialized. @@ -611,16 +679,18 @@ class Map { const_iterator find(const Key& k, TreeIterator* it) const { return FindHelper(k, it).first; } - std::pair FindHelper(const Key& k) const { + template + std::pair FindHelper(const K& k) const { return FindHelper(k, nullptr); } - std::pair FindHelper(const Key& k, + template + std::pair FindHelper(const K& k, TreeIterator* it) const { size_type b = BucketNumber(k); if (TableEntryIsNonEmptyList(b)) { Node* node = static_cast(table_[b]); do { - if (IsMatch(node->kv.first, k)) { + if (internal::TransparentSupport::Equals(node->kv.first, k)) { return std::make_pair(const_iterator(node, this, b), b); } else { node = node->next; @@ -675,9 +745,29 @@ class Map { return result; } + // Returns whether we should insert after the head of the list. For + // non-optimized builds, we randomly decide whether to insert right at the + // head of the list or just after the head. This helps add a little bit of + // non-determinism to the map ordering. + bool ShouldInsertAfterHead(void* node) { +#ifdef NDEBUG + return false; +#else + // Doing modulo with a prime mixes the bits more. + return (reinterpret_cast(node) ^ seed_) % 13 > 6; +#endif + } + // Helper for InsertUnique. Handles the case where bucket b is a // not-too-long linked list. iterator InsertUniqueInList(size_type b, Node* node) { + if (table_[b] != nullptr && ShouldInsertAfterHead(node)) { + Node* first = static_cast(table_[b]); + node->next = first->next; + first->next = node; + return iterator(node, this, b); + } + node->next = static_cast(table_[b]); table_[b] = static_cast(node); return iterator(node, this, b); @@ -767,7 +857,7 @@ class Map { Tree* tree = static_cast(table[index]); typename Tree::iterator tree_it = tree->begin(); do { - InsertUnique(BucketNumber(tree_it->first), + InsertUnique(BucketNumber(std::cref(tree_it->first).get()), NodeFromTreeIterator(tree_it)); } while (++tree_it != tree->end()); DestroyTree(tree); @@ -848,13 +938,19 @@ class Map { return count >= kMaxLength; } - size_type BucketNumber(const Key& k) const { - size_type h = hash_function()(k); - return (h + seed_) & (num_buckets_ - 1); + template + size_type BucketNumber(const K& k) const { + // We xor the hash value against the random seed so that we effectively + // have a random hash function. + uint64 h = hash_function()(k) ^ seed_; + + // We use the multiplication method to determine the bucket number from + // the hash value. The constant kPhi (suggested by Knuth) is roughly + // (sqrt(5) - 1) / 2 * 2^64. + constexpr uint64 kPhi = uint64{0x9e3779b97f4a7c15}; + return ((kPhi * h) >> 32) & (num_buckets_ - 1); } - bool IsMatch(const Key& k0, const Key& k1) const { return k0 == k1; } - // Return a power of two no less than max(kMinTableSize, n). // Assumes either n < kMinTableSize or n is a power of two. size_type TableSize(size_type n) { @@ -899,7 +995,10 @@ class Map { // Return a randomish value. size_type Seed() const { - size_type s = static_cast(reinterpret_cast(this)); + // We get a little bit of randomness from the address of the map. The + // lower bits are not very random, due to alignment, so we discard them + // and shift the higher bits into their place. + size_type s = reinterpret_cast(this) >> 12; #if defined(__x86_64__) && defined(__GNUC__) && \ !defined(GOOGLE_PROTOBUF_NO_RDTSC) uint32 hi, lo; @@ -922,6 +1021,10 @@ class Map { GOOGLE_DISALLOW_EVIL_CONSTRUCTORS(InnerMap); }; // end of class InnerMap + template + using key_arg = typename internal::TransparentSupport< + key_type>::template key_arg; + public: // Iterators class const_iterator { @@ -1014,30 +1117,44 @@ class Map { // Element access T& operator[](const key_type& key) { return (*elements_)[key].second; } - const T& at(const key_type& key) const { + + template + const T& at(const key_arg& key) const { const_iterator it = find(key); - GOOGLE_CHECK(it != end()) << "key not found: " << key; + GOOGLE_CHECK(it != end()) << "key not found: " << static_cast(key); return it->second; } - T& at(const key_type& key) { + + template + T& at(const key_arg& key) { iterator it = find(key); - GOOGLE_CHECK(it != end()) << "key not found: " << key; + GOOGLE_CHECK(it != end()) << "key not found: " << static_cast(key); return it->second; } // Lookup - size_type count(const key_type& key) const { - const_iterator it = find(key); - GOOGLE_DCHECK(it == end() || key == it->first); - return it == end() ? 0 : 1; + template + size_type count(const key_arg& key) const { + return find(key) == end() ? 0 : 1; } - const_iterator find(const key_type& key) const { + + template + const_iterator find(const key_arg& key) const { return const_iterator(iterator(elements_->find(key))); } - iterator find(const key_type& key) { return iterator(elements_->find(key)); } - bool contains(const Key& key) const { return elements_->contains(key); } + template + iterator find(const key_arg& key) { + return iterator(elements_->find(key)); + } + + template + bool contains(const key_arg& key) const { + return find(key) != end(); + } + + template std::pair equal_range( - const key_type& key) const { + const key_arg& key) const { const_iterator it = find(key); if (it == end()) { return std::pair(it, it); @@ -1046,7 +1163,9 @@ class Map { return std::pair(begin, it); } } - std::pair equal_range(const key_type& key) { + + template + std::pair equal_range(const key_arg& key) { iterator it = find(key); if (it == end()) { return std::pair(it, it); @@ -1079,7 +1198,8 @@ class Map { } // Erase and clear - size_type erase(const key_type& key) { + template + size_type erase(const key_arg& key) { iterator it = find(key); if (it == end()) { return 0; diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index 6deb35f5b3..ab88dd03d7 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -980,6 +980,107 @@ TEST_F(MapImplTest, CopyAssignMapIterator) { EXPECT_EQ(it1.GetKey().GetInt32Value(), it2.GetKey().GetInt32Value()); } +// Attempts to verify that a map with keys a and b has a random ordering. This +// function returns true if it succeeds in observing both possible orderings. +bool MapOrderingIsRandom(int a, int b) { + bool saw_a_first = false; + bool saw_b_first = false; + + for (int i = 0; i < 50; ++i) { + Map m; + m[a] = 0; + m[b] = 0; + int32 first_element = m.begin()->first; + if (first_element == a) saw_a_first = true; + if (first_element == b) saw_b_first = true; + if (saw_a_first && saw_b_first) { + return true; + } + } + + return false; +} + +// This test verifies that the iteration order is reasonably random even for +// small maps. Currently we only have sufficient randomness for debug builds and +// builds where we can use the RDTSC instruction, so we only test for those +// builds. +#if defined(__x86_64__) && defined(__GNUC__) && \ + !defined(GOOGLE_PROTOBUF_NO_RDTSC) +TEST_F(MapImplTest, RandomOrdering) { + for (int i = 0; i < 10; ++i) { + for (int j = i + 1; j < 10; ++j) { + EXPECT_TRUE(MapOrderingIsRandom(i, j)) + << "Map with keys " << i << " and " << j + << " has deterministic ordering"; + } + } +} +#endif + +template +void TestTransparent(const Key& key, const Key& miss_key) { + Map m; + const auto& cm = m; + + m.insert({"ABC", 1}); + + const auto abc_it = m.begin(); + + m.insert({"DEF", 2}); + + using testing::Pair; + using testing::UnorderedElementsAre; + + EXPECT_EQ(m.at(key), 1); + EXPECT_EQ(cm.at(key), 1); + +#ifdef PROTOBUF_HAS_DEATH_TEST + EXPECT_DEATH(m.at(miss_key), ""); + EXPECT_DEATH(cm.at(miss_key), ""); +#endif // PROTOBUF_HAS_DEATH_TEST + + EXPECT_EQ(m.count(key), 1); + EXPECT_EQ(cm.count(key), 1); + EXPECT_EQ(m.count(miss_key), 0); + EXPECT_EQ(cm.count(miss_key), 0); + + EXPECT_EQ(m.find(key), abc_it); + EXPECT_EQ(cm.find(key), abc_it); + EXPECT_EQ(m.find(miss_key), m.end()); + EXPECT_EQ(cm.find(miss_key), cm.end()); + + EXPECT_TRUE(m.contains(key)); + EXPECT_TRUE(cm.contains(key)); + EXPECT_FALSE(m.contains(miss_key)); + EXPECT_FALSE(cm.contains(miss_key)); + + EXPECT_THAT(m.equal_range(key), Pair(abc_it, std::next(abc_it))); + EXPECT_THAT(cm.equal_range(key), Pair(abc_it, std::next(abc_it))); + EXPECT_THAT(m.equal_range(miss_key), Pair(m.end(), m.end())); + EXPECT_THAT(cm.equal_range(miss_key), Pair(m.end(), m.end())); + + EXPECT_THAT(m, UnorderedElementsAre(Pair("ABC", 1), Pair("DEF", 2))); + EXPECT_EQ(m.erase(key), 1); + EXPECT_THAT(m, UnorderedElementsAre(Pair("DEF", 2))); + EXPECT_EQ(m.erase(key), 0); + EXPECT_EQ(m.erase(miss_key), 0); + EXPECT_THAT(m, UnorderedElementsAre(Pair("DEF", 2))); +} + +TEST_F(MapImplTest, TransparentLookupForString) { + TestTransparent("ABC", "LKJ"); + TestTransparent(std::string("ABC"), std::string("LKJ")); +#if defined(__cpp_lib_string_view) + TestTransparent(std::string_view("ABC"), std::string_view("LKJ")); +#endif // defined(__cpp_lib_string_view) + + // std::reference_wrapper + std::string abc = "ABC", lkj = "LKJ"; + TestTransparent(std::ref(abc), std::ref(lkj)); + TestTransparent(std::cref(abc), std::cref(lkj)); +} + // Map Field Reflection Test ======================================== static int Func(int i, int j) { return i * j; } @@ -2408,45 +2509,45 @@ TEST(GeneratedMapFieldTest, IsInitialized) { TEST(GeneratedMapFieldTest, MessagesMustMerge) { unittest::TestRequiredMessageMap map_message; + unittest::TestRequired with_dummy4; with_dummy4.set_a(97); - with_dummy4.set_b(0); - with_dummy4.set_c(0); + with_dummy4.set_b(91); with_dummy4.set_dummy4(98); - - EXPECT_TRUE(with_dummy4.IsInitialized()); + EXPECT_FALSE(with_dummy4.IsInitialized()); (*map_message.mutable_map_field())[0] = with_dummy4; - EXPECT_TRUE(map_message.IsInitialized()); - std::string s = map_message.SerializeAsString(); + EXPECT_FALSE(map_message.IsInitialized()); - // Modify s so that there are two values in the entry for key 0. - // The first will have no value for c. The second will have no value for a. - // Those are required fields. Also, make some other little changes, to - // ensure we are merging the two values (because they're messages). - ASSERT_EQ(s.size() - 2, s[1]); // encoding of the length of what follows - std::string encoded_val(s.data() + 4, s.data() + s.size()); - // In s, change the encoding of c to an encoding of dummy32. - s[s.size() - 3] -= 8; - // Make encoded_val slightly different from what's in s. - encoded_val[encoded_val.size() - 1] += 33; // Encode c = 33. - for (int i = 0; i < encoded_val.size(); i++) { - if (encoded_val[i] == 97) { - // Encode b = 91 instead of a = 97. But this won't matter, because - // we also encode b = 0 right after this. The point is to leave out - // a required field, and make sure the parser doesn't complain, because - // every required field is set after the merge of the two values. - encoded_val[i - 1] += 16; - encoded_val[i] = 91; - } else if (encoded_val[i] == 98) { - // Encode dummy5 = 99 instead of dummy4 = 98. - encoded_val[i - 1] += 8; // The tag for dummy5 is 8 more. - encoded_val[i]++; - break; - } - } + unittest::TestRequired with_dummy5; + with_dummy5.set_b(0); + with_dummy5.set_c(33); + with_dummy5.set_dummy5(99); + EXPECT_FALSE(with_dummy5.IsInitialized()); + (*map_message.mutable_map_field())[0] = with_dummy5; + EXPECT_FALSE(map_message.IsInitialized()); - s += encoded_val; // Add the second message. - s[1] += encoded_val.size(); // Adjust encoded size. + // The wire format of MapEntry is straightforward (*) and can be manually + // constructed to force merging of two uninitialized messages that would + // result in an initialized message. + // + // (*) http://google3/net/proto2/internal/map_test.cc?l=2433&rcl=310012028 + std::string dummy4_s = with_dummy4.SerializePartialAsString(); + std::string dummy5_s = with_dummy5.SerializePartialAsString(); + int payload_size = dummy4_s.size() + dummy5_s.size(); + // Makes sure the payload size fits into one byte. + ASSERT_LT(payload_size, 128); + + std::string s(6, 0); + char* p = &s[0]; + *p++ = WireFormatLite::MakeTag(1, WireFormatLite::WIRETYPE_LENGTH_DELIMITED); + // Length: 2B for key tag & val and 2B for val tag and length of the following + // payload. + *p++ = 4 + payload_size; + *p++ = WireFormatLite::MakeTag(1, WireFormatLite::WIRETYPE_VARINT); + *p++ = 0; + *p++ = WireFormatLite::MakeTag(2, WireFormatLite::WIRETYPE_LENGTH_DELIMITED); + *p++ = payload_size; + StrAppend(&s, dummy4_s, dummy5_s); // Test key then value then value. int key = 0; @@ -2983,10 +3084,9 @@ TEST(WireFormatForMapFieldTest, SerializeMap) { ASSERT_FALSE(output.HadError()); } - // Should be the same. - // Don't use EXPECT_EQ here because we're comparing raw binary data and - // we really don't want it dumped to stdout on failure. - EXPECT_TRUE(dynamic_data == generated_data); + // Should parse to the same message. + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, generated_data)); + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, dynamic_data)); } TEST(WireFormatForMapFieldTest, SerializeMapDynamicMessage) { @@ -3161,20 +3261,6 @@ static std::string DeterministicSerialization(const T& t) { return result; } -// Helper to test the serialization of the first arg against a golden file. -static void TestDeterministicSerialization(const protobuf_unittest::TestMaps& t, - const std::string& filename) { - std::string expected; - GOOGLE_CHECK_OK(File::GetContents( - TestUtil::GetTestDataPath("net/proto2/internal/testdata/" + filename), - &expected, true)); - const std::string actual = DeterministicSerialization(t); - EXPECT_EQ(expected, actual); - protobuf_unittest::TestMaps u; - EXPECT_TRUE(u.ParseFromString(actual)); - EXPECT_TRUE(util::MessageDifferencer::Equals(u, t)); -} - // Helper for MapSerializationTest. Return a 7-bit ASCII string. static std::string ConstructKey(uint64 n) { std::string s(n % static_cast(9), '\0'); @@ -3219,7 +3305,17 @@ TEST(MapSerializationTest, Deterministic) { frog = frog * multiplier + i; frog ^= (frog >> 41); } - TestDeterministicSerialization(t, "golden_message_maps"); + + // Verifies if two consecutive calls to deterministic serialization produce + // the same bytes. Deterministic serialization means the same serialization + // bytes in the same binary. + const std::string s1 = DeterministicSerialization(t); + const std::string s2 = DeterministicSerialization(t); + EXPECT_EQ(s1, s2); + + protobuf_unittest::TestMaps u; + EXPECT_TRUE(u.ParseFromString(s1)); + EXPECT_TRUE(util::MessageDifferencer::Equals(u, t)); } TEST(MapSerializationTest, DeterministicSubmessage) { diff --git a/src/google/protobuf/map_test_util_impl.h b/src/google/protobuf/map_test_util_impl.h index 42452d2276..655aec8384 100644 --- a/src/google/protobuf/map_test_util_impl.h +++ b/src/google/protobuf/map_test_util_impl.h @@ -405,7 +405,7 @@ void MapTestUtilImpl::ExpectMapFieldsSetInitialized(const MapMessage& message) { EXPECT_EQ("", message.map_string_string().at("0")); EXPECT_EQ("", message.map_int32_bytes().at(0)); EXPECT_EQ(enum_value, message.map_int32_enum().at(0)); - EXPECT_EQ(0, message.map_int32_foreign_message().at(0).ByteSize()); + EXPECT_EQ(0, message.map_int32_foreign_message().at(0).ByteSizeLong()); } template AddVarint(1, 2); // And the unknown fields should be changed. - ASSERT_NE(original.ByteSize(), arena_message->ByteSize()); + ASSERT_NE(original.ByteSizeLong(), arena_message->ByteSizeLong()); ASSERT_FALSE( arena_message->GetReflection()->GetUnknownFields(*arena_message).empty()); } @@ -203,7 +203,7 @@ TEST(Proto3OptionalTest, OptionalFields) { msg.set_optional_int32(0); EXPECT_TRUE(msg.has_optional_int32()); - string serialized; + std::string serialized; msg.SerializeToString(&serialized); EXPECT_GT(serialized.size(), 0); @@ -236,7 +236,7 @@ TEST(Proto3OptionalTest, OptionalField) { msg.set_optional_int32(0); EXPECT_TRUE(msg.has_optional_int32()); - string serialized; + std::string serialized; msg.SerializeToString(&serialized); EXPECT_GT(serialized.size(), 0); diff --git a/src/google/protobuf/struct.pb.h b/src/google/protobuf/struct.pb.h index 078ec44436..671a0cb0cd 100644 --- a/src/google/protobuf/struct.pb.h +++ b/src/google/protobuf/struct.pb.h @@ -100,7 +100,7 @@ inline const std::string& NullValue_Name(T enum_t_value) { NullValue_descriptor(), enum_t_value); } inline bool NullValue_Parse( - const std::string& name, NullValue* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, NullValue* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( NullValue_descriptor(), name, value); } diff --git a/src/google/protobuf/stubs/port.h b/src/google/protobuf/stubs/port.h index c21c28a70f..3c7ada385c 100644 --- a/src/google/protobuf/stubs/port.h +++ b/src/google/protobuf/stubs/port.h @@ -117,6 +117,8 @@ namespace google { namespace protobuf { +using ConstStringParam = const std::string &; + typedef unsigned int uint; typedef int8_t int8; diff --git a/src/google/protobuf/test_util_lite.cc b/src/google/protobuf/test_util_lite.cc index a897fe26b5..e7eb60a6b7 100644 --- a/src/google/protobuf/test_util_lite.cc +++ b/src/google/protobuf/test_util_lite.cc @@ -1351,7 +1351,7 @@ void TestUtilLite::ExpectExtensionsClear( std::string serialized; ASSERT_TRUE(message.SerializeToString(&serialized)); EXPECT_EQ("", serialized); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); // has_blah() should initially be false for all optional fields. EXPECT_FALSE(message.HasExtension(unittest::optional_int32_extension_lite)); diff --git a/src/google/protobuf/text_format_unittest.cc b/src/google/protobuf/text_format_unittest.cc index 9e7479e63d..0ae2def2dc 100644 --- a/src/google/protobuf/text_format_unittest.cc +++ b/src/google/protobuf/text_format_unittest.cc @@ -55,6 +55,7 @@ #include #include #include +#include #include #include #include @@ -346,14 +347,13 @@ TEST_F(TextFormatTest, PrintUnknownMessage) { UnknownFieldSet unknown_fields; EXPECT_TRUE(unknown_fields.ParseFromString(data)); EXPECT_TRUE(TextFormat::PrintUnknownFieldsToString(unknown_fields, &text)); - EXPECT_EQ( - "44: \"abc\"\n" - "44: \"def\"\n" - "44: \"\"\n" - "48 {\n" - " 1: 123\n" - "}\n", - text); + // Field 44 and 48 can be printed in any order. + EXPECT_THAT(text, testing::HasSubstr("44: \"abc\"\n" + "44: \"def\"\n" + "44: \"\"\n")); + EXPECT_THAT(text, testing::HasSubstr("48 {\n" + " 1: 123\n" + "}\n")); } TEST_F(TextFormatTest, PrintDeeplyNestedUnknownMessage) { diff --git a/src/google/protobuf/type.pb.h b/src/google/protobuf/type.pb.h index 9b341fc045..eb4b937165 100644 --- a/src/google/protobuf/type.pb.h +++ b/src/google/protobuf/type.pb.h @@ -121,7 +121,7 @@ inline const std::string& Field_Kind_Name(T enum_t_value) { Field_Kind_descriptor(), enum_t_value); } inline bool Field_Kind_Parse( - const std::string& name, Field_Kind* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Field_Kind* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( Field_Kind_descriptor(), name, value); } @@ -148,7 +148,7 @@ inline const std::string& Field_Cardinality_Name(T enum_t_value) { Field_Cardinality_descriptor(), enum_t_value); } inline bool Field_Cardinality_Parse( - const std::string& name, Field_Cardinality* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Field_Cardinality* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( Field_Cardinality_descriptor(), name, value); } @@ -173,7 +173,7 @@ inline const std::string& Syntax_Name(T enum_t_value) { Syntax_descriptor(), enum_t_value); } inline bool Syntax_Parse( - const std::string& name, Syntax* value) { + ::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Syntax* value) { return ::PROTOBUF_NAMESPACE_ID::internal::ParseNamedEnum( Syntax_descriptor(), name, value); } @@ -598,7 +598,7 @@ class PROTOBUF_EXPORT Field PROTOBUF_FINAL : "Incorrect type passed to function Kind_Name."); return Field_Kind_Name(enum_t_value); } - static inline bool Kind_Parse(const std::string& name, + static inline bool Kind_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Kind* value) { return Field_Kind_Parse(name, value); } @@ -632,7 +632,7 @@ class PROTOBUF_EXPORT Field PROTOBUF_FINAL : "Incorrect type passed to function Cardinality_Name."); return Field_Cardinality_Name(enum_t_value); } - static inline bool Cardinality_Parse(const std::string& name, + static inline bool Cardinality_Parse(::PROTOBUF_NAMESPACE_ID::ConstStringParam name, Cardinality* value) { return Field_Cardinality_Parse(name, value); } diff --git a/src/google/protobuf/unknown_field_set_unittest.cc b/src/google/protobuf/unknown_field_set_unittest.cc index 0083208e47..bc8db5f514 100644 --- a/src/google/protobuf/unknown_field_set_unittest.cc +++ b/src/google/protobuf/unknown_field_set_unittest.cc @@ -37,6 +37,8 @@ #include +#include + #include #include #include @@ -113,30 +115,32 @@ class UnknownFieldSetTest : public testing::Test { namespace { TEST_F(UnknownFieldSetTest, AllFieldsPresent) { - // All fields of TestAllTypes should be present, in numeric order (because - // that's the order we parsed them in). Fields that are not valid field - // numbers of TestAllTypes should NOT be present. - - int pos = 0; - - for (int i = 0; i < 1000; i++) { - const FieldDescriptor* field = descriptor_->FindFieldByNumber(i); - if (field != NULL) { - ASSERT_LT(pos, unknown_fields_->field_count()); - // Do not check oneof field if it is not set. - if (field->containing_oneof() == NULL) { - EXPECT_EQ(i, unknown_fields_->field(pos++).number()); - } else if (i == unknown_fields_->field(pos).number()) { - pos++; - } - if (field->is_repeated()) { - // Should have a second instance. - ASSERT_LT(pos, unknown_fields_->field_count()); - EXPECT_EQ(i, unknown_fields_->field(pos++).number()); - } + // Verifies the following: + // --all unknown tags belong to TestAllTypes. + // --all fields in TestAllTypes is present in UnknownFieldSet except unset + // oneof fields. + // + // Should handle repeated fields that may appear multiple times in + // UnknownFieldSet. + + int non_oneof_count = 0; + for (int i = 0; i < descriptor_->field_count(); i++) { + if (!descriptor_->field(i)->containing_oneof()) { + non_oneof_count++; } } - EXPECT_EQ(unknown_fields_->field_count(), pos); + + std::unordered_set unknown_tags; + for (int i = 0; i < unknown_fields_->field_count(); i++) { + unknown_tags.insert(unknown_fields_->field(i).number()); + } + + for (uint32 t : unknown_tags) { + EXPECT_NE(descriptor_->FindFieldByNumber(t), nullptr); + } + + EXPECT_EQ(non_oneof_count + descriptor_->oneof_decl_count(), + unknown_tags.size()); } TEST_F(UnknownFieldSetTest, Varint) { @@ -246,7 +250,7 @@ TEST_F(UnknownFieldSetTest, SerializeViaReflection) { { io::StringOutputStream raw_output(&data); io::CodedOutputStream output(&raw_output); - int size = WireFormat::ByteSize(empty_message_); + size_t size = WireFormat::ByteSize(empty_message_); WireFormat::SerializeWithCachedSizes(empty_message_, size, &output); ASSERT_FALSE(output.HadError()); } @@ -536,29 +540,29 @@ TEST_F(UnknownFieldSetTest, SpaceUsed) { // Make sure an unknown field set has zero space used until a field is // actually added. - int base_size = empty_message.SpaceUsed(); + size_t base_size = empty_message.SpaceUsedLong(); UnknownFieldSet* unknown_fields = empty_message.mutable_unknown_fields(); - EXPECT_EQ(base_size, empty_message.SpaceUsed()); + EXPECT_EQ(base_size, empty_message.SpaceUsedLong()); - // Make sure each thing we add to the set increases the SpaceUsed(). + // Make sure each thing we add to the set increases the SpaceUsedLong(). unknown_fields->AddVarint(1, 0); - EXPECT_LT(base_size, empty_message.SpaceUsed()); - base_size = empty_message.SpaceUsed(); + EXPECT_LT(base_size, empty_message.SpaceUsedLong()); + base_size = empty_message.SpaceUsedLong(); std::string* str = unknown_fields->AddLengthDelimited(1); - EXPECT_LT(base_size, empty_message.SpaceUsed()); - base_size = empty_message.SpaceUsed(); + EXPECT_LT(base_size, empty_message.SpaceUsedLong()); + base_size = empty_message.SpaceUsedLong(); str->assign(sizeof(std::string) + 1, 'x'); - EXPECT_LT(base_size, empty_message.SpaceUsed()); - base_size = empty_message.SpaceUsed(); + EXPECT_LT(base_size, empty_message.SpaceUsedLong()); + base_size = empty_message.SpaceUsedLong(); UnknownFieldSet* group = unknown_fields->AddGroup(1); - EXPECT_LT(base_size, empty_message.SpaceUsed()); - base_size = empty_message.SpaceUsed(); + EXPECT_LT(base_size, empty_message.SpaceUsedLong()); + base_size = empty_message.SpaceUsedLong(); group->AddVarint(1, 0); - EXPECT_LT(base_size, empty_message.SpaceUsed()); + EXPECT_LT(base_size, empty_message.SpaceUsedLong()); } diff --git a/src/google/protobuf/util/field_mask_util_test.cc b/src/google/protobuf/util/field_mask_util_test.cc index 5fe9f65bb7..9ed35e643b 100644 --- a/src/google/protobuf/util/field_mask_util_test.cc +++ b/src/google/protobuf/util/field_mask_util_test.cc @@ -46,7 +46,7 @@ namespace util { class SnakeCaseCamelCaseTest : public ::testing::Test { protected: - string SnakeCaseToCamelCase(const std::string& input) { + std::string SnakeCaseToCamelCase(const std::string& input) { std::string output; if (FieldMaskUtil::SnakeCaseToCamelCase(input, &output)) { return output; @@ -55,7 +55,7 @@ class SnakeCaseCamelCaseTest : public ::testing::Test { } } - string CamelCaseToSnakeCase(const std::string& input) { + std::string CamelCaseToSnakeCase(const std::string& input) { std::string output; if (FieldMaskUtil::CamelCaseToSnakeCase(input, &output)) { return output; diff --git a/src/google/protobuf/util/json_util.cc b/src/google/protobuf/util/json_util.cc index 2f3e6c6ba5..6ced27c465 100644 --- a/src/google/protobuf/util/json_util.cc +++ b/src/google/protobuf/util/json_util.cc @@ -145,8 +145,8 @@ class StatusErrorListener : public converter::ErrorListener { StringPiece value) override { status_ = util::Status( util::error::INVALID_ARGUMENT, - StrCat(GetLocString(loc), ": invalid value ", string(value), - " for type ", string(type_name))); + StrCat(GetLocString(loc), ": invalid value ", std::string(value), + " for type ", std::string(type_name))); } void MissingField(const converter::LocationTrackerInterface& loc, diff --git a/src/google/protobuf/util/message_differencer_unittest.cc b/src/google/protobuf/util/message_differencer_unittest.cc index 637d4d3c0c..2f6be98ccd 100644 --- a/src/google/protobuf/util/message_differencer_unittest.cc +++ b/src/google/protobuf/util/message_differencer_unittest.cc @@ -1205,7 +1205,7 @@ TEST(MessageDifferencerTest, RepeatedFieldSmartSetTest_PreviouslyMatch) { *msg2.add_rm() = elem1_2; *msg2.add_rm() = elem2_2; - string diff_report; + std::string diff_report; util::MessageDifferencer differencer; differencer.ReportDifferencesToString(&diff_report); differencer.set_repeated_field_comparison( @@ -2385,7 +2385,7 @@ class ComparisonTest : public testing::Test { void field_as_set(const std::string& field) { set_field_ = field; } - void field_as_map(const string& field, const std::string& key) { + void field_as_map(const std::string& field, const std::string& key) { map_field_ = field; map_key_ = key; } @@ -3198,11 +3198,13 @@ TEST_F(ComparisonTest, EquivalentIgnoresUnknown) { } TEST_F(ComparisonTest, MapTest) { - Map& map1 = *map_proto1_.mutable_map_string_string(); + Map& map1 = + *map_proto1_.mutable_map_string_string(); map1["key1"] = "1"; map1["key2"] = "2"; map1["key3"] = "3"; - Map& map2 = *map_proto2_.mutable_map_string_string(); + Map& map2 = + *map_proto2_.mutable_map_string_string(); map2["key3"] = "0"; map2["key2"] = "2"; map2["key1"] = "1"; @@ -3212,11 +3214,13 @@ TEST_F(ComparisonTest, MapTest) { } TEST_F(ComparisonTest, MapIgnoreKeyTest) { - Map& map1 = *map_proto1_.mutable_map_string_string(); + Map& map1 = + *map_proto1_.mutable_map_string_string(); map1["key1"] = "1"; map1["key2"] = "2"; map1["key3"] = "3"; - Map& map2 = *map_proto2_.mutable_map_string_string(); + Map& map2 = + *map_proto2_.mutable_map_string_string(); map2["key4"] = "2"; map2["key5"] = "3"; map2["key6"] = "1"; diff --git a/src/google/protobuf/util/type_resolver_util.cc b/src/google/protobuf/util/type_resolver_util.cc index 35736f1007..e906d58d70 100644 --- a/src/google/protobuf/util/type_resolver_util.cc +++ b/src/google/protobuf/util/type_resolver_util.cc @@ -305,7 +305,7 @@ class DescriptorPoolTypeResolver : public TypeResolver { return url_prefix_ + "/" + descriptor->full_name(); } - Status ParseTypeUrl(const string& type_url, std::string* type_name) { + Status ParseTypeUrl(const std::string& type_url, std::string* type_name) { if (type_url.substr(0, url_prefix_.size() + 1) != url_prefix_ + "/") { return Status( util::error::INVALID_ARGUMENT, diff --git a/src/google/protobuf/util/type_resolver_util_test.cc b/src/google/protobuf/util/type_resolver_util_test.cc index f9b9ad65a1..d90fdffd9e 100644 --- a/src/google/protobuf/util/type_resolver_util_test.cc +++ b/src/google/protobuf/util/type_resolver_util_test.cc @@ -158,7 +158,7 @@ class DescriptorPoolTypeResolverTest : public testing::Test { return false; } - string GetTypeUrl(std::string full_name) { + std::string GetTypeUrl(std::string full_name) { return kUrlPrefix + std::string("/") + full_name; } diff --git a/src/google/protobuf/wire_format_unittest.cc b/src/google/protobuf/wire_format_unittest.cc index 24cb58f629..fc9e6a5e6f 100644 --- a/src/google/protobuf/wire_format_unittest.cc +++ b/src/google/protobuf/wire_format_unittest.cc @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -48,12 +49,15 @@ #include #include #include +#include #include #include #include #include +// clang-format off #include +// clang-format on namespace google { namespace protobuf { @@ -209,8 +213,16 @@ TEST(WireFormatTest, OneofOnlySetLast) { source.set_foo_int(100); source.set_foo_string("101"); - // Serialize and parse to oneof message. - source.SerializeToString(&data); + // Serialize and parse to oneof message. Generated serializer may not order + // fields in tag order. Use WireFormat::SerializeWithCachedSizes instead as + // it sorts fields beforehand. + { + io::StringOutputStream raw_output(&data); + io::CodedOutputStream output(&raw_output); + WireFormat::SerializeWithCachedSizes(source, source.ByteSizeLong(), + &output); + ASSERT_FALSE(output.HadError()); + } io::ArrayInputStream raw_input(data.data(), data.size()); io::CodedInputStream input(&raw_input); WireFormat::ParseAndMergePartial(&input, &oneof_dest); @@ -224,9 +236,9 @@ TEST(WireFormatTest, ByteSize) { unittest::TestAllTypes message; TestUtil::SetAllFields(&message); - EXPECT_EQ(message.ByteSize(), WireFormat::ByteSize(message)); + EXPECT_EQ(message.ByteSizeLong(), WireFormat::ByteSize(message)); message.Clear(); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); EXPECT_EQ(0, WireFormat::ByteSize(message)); } @@ -234,9 +246,9 @@ TEST(WireFormatTest, ByteSizeExtensions) { unittest::TestAllExtensions message; TestUtil::SetAllExtensions(&message); - EXPECT_EQ(message.ByteSize(), WireFormat::ByteSize(message)); + EXPECT_EQ(message.ByteSizeLong(), WireFormat::ByteSize(message)); message.Clear(); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); EXPECT_EQ(0, WireFormat::ByteSize(message)); } @@ -244,9 +256,9 @@ TEST(WireFormatTest, ByteSizePacked) { unittest::TestPackedTypes message; TestUtil::SetPackedFields(&message); - EXPECT_EQ(message.ByteSize(), WireFormat::ByteSize(message)); + EXPECT_EQ(message.ByteSizeLong(), WireFormat::ByteSize(message)); message.Clear(); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); EXPECT_EQ(0, WireFormat::ByteSize(message)); } @@ -254,9 +266,9 @@ TEST(WireFormatTest, ByteSizePackedExtensions) { unittest::TestPackedExtensions message; TestUtil::SetPackedExtensions(&message); - EXPECT_EQ(message.ByteSize(), WireFormat::ByteSize(message)); + EXPECT_EQ(message.ByteSizeLong(), WireFormat::ByteSize(message)); message.Clear(); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); EXPECT_EQ(0, WireFormat::ByteSize(message)); } @@ -264,10 +276,10 @@ TEST(WireFormatTest, ByteSizeOneof) { unittest::TestOneof2 message; TestUtil::SetOneof1(&message); - EXPECT_EQ(message.ByteSize(), WireFormat::ByteSize(message)); + EXPECT_EQ(message.ByteSizeLong(), WireFormat::ByteSize(message)); message.Clear(); - EXPECT_EQ(0, message.ByteSize()); + EXPECT_EQ(0, message.ByteSizeLong()); EXPECT_EQ(0, WireFormat::ByteSize(message)); } @@ -277,7 +289,7 @@ TEST(WireFormatTest, Serialize) { std::string dynamic_data; TestUtil::SetAllFields(&message); - int size = message.ByteSize(); + size_t size = message.ByteSizeLong(); // Serialize using the generated code. { @@ -295,10 +307,9 @@ TEST(WireFormatTest, Serialize) { ASSERT_FALSE(output.HadError()); } - // Should be the same. - // Don't use EXPECT_EQ here because we're comparing raw binary data and - // we really don't want it dumped to stdout on failure. - EXPECT_TRUE(dynamic_data == generated_data); + // Should parse to the same message. + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, generated_data)); + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, dynamic_data)); } TEST(WireFormatTest, SerializeExtensions) { @@ -307,7 +318,7 @@ TEST(WireFormatTest, SerializeExtensions) { std::string dynamic_data; TestUtil::SetAllExtensions(&message); - int size = message.ByteSize(); + size_t size = message.ByteSizeLong(); // Serialize using the generated code. { @@ -325,10 +336,9 @@ TEST(WireFormatTest, SerializeExtensions) { ASSERT_FALSE(output.HadError()); } - // Should be the same. - // Don't use EXPECT_EQ here because we're comparing raw binary data and - // we really don't want it dumped to stdout on failure. - EXPECT_TRUE(dynamic_data == generated_data); + // Should parse to the same message. + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, generated_data)); + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, dynamic_data)); } TEST(WireFormatTest, SerializeFieldsAndExtensions) { @@ -337,7 +347,7 @@ TEST(WireFormatTest, SerializeFieldsAndExtensions) { std::string dynamic_data; TestUtil::SetAllFieldsAndExtensions(&message); - int size = message.ByteSize(); + size_t size = message.ByteSizeLong(); // Serialize using the generated code. { @@ -355,14 +365,9 @@ TEST(WireFormatTest, SerializeFieldsAndExtensions) { ASSERT_FALSE(output.HadError()); } - // Should be the same. - // Don't use EXPECT_EQ here because we're comparing raw binary data and - // we really don't want it dumped to stdout on failure. - EXPECT_TRUE(dynamic_data == generated_data); - - // Should output in canonical order. - TestUtil::ExpectAllFieldsAndExtensionsInOrder(dynamic_data); - TestUtil::ExpectAllFieldsAndExtensionsInOrder(generated_data); + // Should parse to the same message. + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, generated_data)); + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, dynamic_data)); } TEST(WireFormatTest, SerializeOneof) { @@ -371,7 +376,7 @@ TEST(WireFormatTest, SerializeOneof) { std::string dynamic_data; TestUtil::SetOneof1(&message); - int size = message.ByteSize(); + size_t size = message.ByteSizeLong(); // Serialize using the generated code. { @@ -389,10 +394,9 @@ TEST(WireFormatTest, SerializeOneof) { ASSERT_FALSE(output.HadError()); } - // Should be the same. - // Don't use EXPECT_EQ here because we're comparing raw binary data and - // we really don't want it dumped to stdout on failure. - EXPECT_TRUE(dynamic_data == generated_data); + // Should parse to the same message. + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, generated_data)); + EXPECT_TRUE(TestUtil::EqualsToSerialized(message, dynamic_data)); } TEST(WireFormatTest, ParseMultipleExtensionRanges) { @@ -482,7 +486,7 @@ TEST(WireFormatTest, SerializeMessageSetVariousWaysAreEqual) { message_set.mutable_unknown_fields()->AddLengthDelimited(kUnknownTypeId, "bar"); - int size = message_set.ByteSize(); + size_t size = message_set.ByteSizeLong(); EXPECT_EQ(size, message_set.GetCachedSize()); ASSERT_EQ(size, WireFormat::ByteSize(message_set)); @@ -595,7 +599,7 @@ TEST(WireFormatTest, ParseMessageSetWithReverseTagOrder) { WireFormatLite::WriteTag(WireFormatLite::kMessageSetMessageNumber, WireFormatLite::WIRETYPE_LENGTH_DELIMITED, &coded_output); - coded_output.WriteVarint32(message.ByteSize()); + coded_output.WriteVarint32(message.ByteSizeLong()); message.SerializeWithCachedSizes(&coded_output); // Write the type id. uint32 type_id = message.GetDescriptor()->extension(0)->number(); @@ -989,7 +993,7 @@ class Proto3PrimitiveRepeatedWireFormatTest : public ::testing::Test { void TestSerialization(Proto* message, const std::string& expected) { SetProto3PrimitiveRepeatedFields(message); - int size = message->ByteSize(); + size_t size = message->ByteSizeLong(); // Serialize using the generated code. std::string generated_data; @@ -999,7 +1003,7 @@ class Proto3PrimitiveRepeatedWireFormatTest : public ::testing::Test { message->SerializeWithCachedSizes(&output); ASSERT_FALSE(output.HadError()); } - EXPECT_TRUE(expected == generated_data); + EXPECT_TRUE(TestUtil::EqualsToSerialized(*message, generated_data)); // Serialize using the dynamic code. std::string dynamic_data; diff --git a/tests.sh b/tests.sh index 0f8e10c460..59c4162f3f 100755 --- a/tests.sh +++ b/tests.sh @@ -446,6 +446,13 @@ build_ruby27() { build_javascript() { internal_build_cpp + NODE_VERSION=node-v12.16.3-darwin-x64 + NODE_TGZ="$NODE_VERSION.tar.gz" + pushd /tmp + curl -OL https://nodejs.org/dist/v12.16.3/$NODE_TGZ + tar zxvf $NODE_TGZ + export PATH=$PATH:`pwd`/$NODE_VERSION/bin + popd cd js && npm install && npm test && cd .. cd conformance && make test_nodejs && cd .. } @@ -618,6 +625,7 @@ build_php5.6_mac() { # Install PHP curl -s https://php-osx.liip.ch/install.sh | bash -s 5.6 PHP_FOLDER=`find /usr/local -type d -name "php5-5.6*"` # The folder name may change upon time + test ! -z "$PHP_FOLDER" export PATH="$PHP_FOLDER/bin:$PATH" # Install phpunit @@ -693,7 +701,8 @@ build_php7.0_mac() { generate_php_test_proto # Install PHP curl -s https://php-osx.liip.ch/install.sh | bash -s 7.0 - PHP_FOLDER=`find /usr/local -type d -name "php7-7.0*"` # The folder name may change upon time + PHP_FOLDER=`find /usr/local -type d -name "php5-7.0*"` # The folder name may change upon time + test ! -z "$PHP_FOLDER" export PATH="$PHP_FOLDER/bin:$PATH" # Install phpunit @@ -713,11 +722,14 @@ build_php7.0_mac() { popd } -build_php7.4_mac() { +build_php7.3_mac() { generate_php_test_proto # Install PHP - curl -s https://php-osx.liip.ch/install.sh | bash -s 7.4 - PHP_FOLDER=`find /usr/local -type d -name "php7-7.4*"` # The folder name may change upon time + # We can't test PHP 7.4 with these binaries yet: + # https://github.com/liip/php-osx/issues/276 + curl -s https://php-osx.liip.ch/install.sh | bash -s 7.3 + PHP_FOLDER=`find /usr/local -type d -name "php5-7.3*"` # The folder name may change upon time + test ! -z "$PHP_FOLDER" export PATH="$PHP_FOLDER/bin:$PATH" # Install phpunit