From 3b156187b88a32d51506a4f1b0ecabe360b0d145 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 16 Feb 2018 15:18:16 -0500 Subject: [PATCH 1/2] [En|De]codeUnsignedVarint -> [En|De]codeVarint The 'unsigned' portion was misleading, as these varints work with both signed and unsigned integers. (The 'signed' varints also work with both signed and unsigned integers, but use zig-zag encoding so that negative numbers are encoded more efficiently. Note that 'signed' varints aren't used in the Value proto, so won't appear in the serializer class for at least the short term.) Added some docstrings to help disambiguate this. --- .../firebase/firestore/remote/serializer.cc | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index c6c699f8c8f..8589039bfe6 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -25,9 +25,14 @@ namespace remote { namespace { -void EncodeUnsignedVarint(pb_ostream_t* stream, - uint32_t field_number, - uint64_t value) { +/** + * Encodes a varint. Note that (despite the value parameter type) this works for + * bool, enum, int32, int64, uint32 and uint64 proto field types. + * + * @param value The value to encode. This could be a bool, enum, int32, int64, + * uint32 or uint64 proto field type (represented as a uint64_t). + */ +void EncodeVarint(pb_ostream_t* stream, uint32_t field_number, uint64_t value) { bool status = pb_encode_tag(stream, PB_WT_VARINT, field_number); if (!status) { // TODO(rsgowman): figure out error handling @@ -41,7 +46,14 @@ void EncodeUnsignedVarint(pb_ostream_t* stream, } } -uint64_t DecodeUnsignedVarint(pb_istream_t* stream) { +/** + * Decodes a varint. Note that (despite the return type) this works for bool, + * enum, int32, int64, uint32 and uint64 proto field types. + * + * @return The decoded varint as a uint64_t (though might actually be a bool, + * enum int32, int64, uint32 or uint64 proto field type.) + */ +uint64_t DecodeVarint(pb_istream_t* stream) { uint64_t varint_value; bool status = pb_decode_varint(stream, &varint_value); if (!status) { @@ -52,13 +64,12 @@ uint64_t DecodeUnsignedVarint(pb_istream_t* stream) { } void EncodeNull(pb_ostream_t* stream) { - return EncodeUnsignedVarint(stream, - google_firestore_v1beta1_Value_null_value_tag, - google_protobuf_NullValue_NULL_VALUE); + return EncodeVarint(stream, google_firestore_v1beta1_Value_null_value_tag, + google_protobuf_NullValue_NULL_VALUE); } void DecodeNull(pb_istream_t* stream) { - uint64_t varint = DecodeUnsignedVarint(stream); + uint64_t varint = DecodeVarint(stream); if (varint != google_protobuf_NullValue_NULL_VALUE) { // TODO(rsgowman): figure out error handling abort(); @@ -66,12 +77,12 @@ void DecodeNull(pb_istream_t* stream) { } void EncodeBool(pb_ostream_t* stream, bool bool_value) { - return EncodeUnsignedVarint( - stream, google_firestore_v1beta1_Value_boolean_value_tag, bool_value); + return EncodeVarint(stream, google_firestore_v1beta1_Value_boolean_value_tag, + bool_value); } bool DecodeBool(pb_istream_t* stream) { - uint64_t varint = DecodeUnsignedVarint(stream); + uint64_t varint = DecodeVarint(stream); switch (varint) { case 0: return false; From bf25d9d90ca1d6f7cbfc185be0b160a6fd206338 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 20 Feb 2018 13:51:01 -0500 Subject: [PATCH 2/2] review feedback --- .../firebase/firestore/remote/serializer.cc | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 8589039bfe6..fda257bdf57 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -26,11 +26,13 @@ namespace remote { namespace { /** - * Encodes a varint. Note that (despite the value parameter type) this works for - * bool, enum, int32, int64, uint32 and uint64 proto field types. + * Note that (despite the value parameter type) this works for bool, enum, + * int32, int64, uint32 and uint64 proto field types. * - * @param value The value to encode. This could be a bool, enum, int32, int64, - * uint32 or uint64 proto field type (represented as a uint64_t). + * Note: This is not expected to be called direclty, but rather only via the + * other Encode* methods (i.e. EncodeBool, EncodeLong, etc) + * + * @param value The value to encode, represented as a uint64_t. */ void EncodeVarint(pb_ostream_t* stream, uint32_t field_number, uint64_t value) { bool status = pb_encode_tag(stream, PB_WT_VARINT, field_number); @@ -47,11 +49,13 @@ void EncodeVarint(pb_ostream_t* stream, uint32_t field_number, uint64_t value) { } /** - * Decodes a varint. Note that (despite the return type) this works for bool, - * enum, int32, int64, uint32 and uint64 proto field types. + * Note that (despite the return type) this works for bool, enum, int32, int64, + * uint32 and uint64 proto field types. + * + * Note: This is not expected to be called direclty, but rather only via the + * other Decode* methods (i.e. DecodeBool, DecodeLong, etc) * - * @return The decoded varint as a uint64_t (though might actually be a bool, - * enum int32, int64, uint32 or uint64 proto field type.) + * @return The decoded varint as a uint64_t. */ uint64_t DecodeVarint(pb_istream_t* stream) { uint64_t varint_value;