[En|De]codeUnsignedVarint -> [En|De]codeVarint

[rsgowman]

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.

[var-const]

1. Perhaps make it a template function? Or are the proto fields which are passed to this function already of type uint64_t? This would slightly increase code size, but the advantage is that this weird quirk of Nanopb will be hidden in the implementation.

2. It sounds like Nanopb relies on negative int-to-uint conversion?

3. Consider trimming down the comment a little bit:

• Personally, I think that "Encodes a varint" pretty much repeats the function name and can be omitted
• The part specifying the range of acceptable integer-like values is repeated twice.

4. Nit: I suppose it would work for any int-based type <=64 (char, int16_t, etc.)?

[rsgowman]

Perhaps make it a template function? Or are the proto fields which are passed to this function already of type uint64_t? This would slightly increase code size, but the advantage is that this weird quirk of Nanopb will be hidden in the implementation.

Well, we're already hiding the implementation. Note that EncodeVarint is only called via the other Encode* methods such as EncodeNull, EncodeBool, EncodeLong (not yet present, but see next PR), etc. I suppose those methods could be replaced with a template function though... (possibly excepting EncodeNull).

In any case, the "called via the other Encode* methods" could be made a bit more explicit via a comment, so I've added that for now. I'm indifferent re the template function... we may have to specialize them all anyway due to the tag (though that only applies to Encode*). I've left as is for now, but am open to switching.

It sounds like Nanopb relies on negative int-to-uint conversion?

Yeah. Also see next PR (not yet published) for more explicit tests of this.

Personally, I think that "Encodes a varint" pretty much repeats the function name and can be omitted

Done.

The part specifying the range of acceptable integer-like values is repeated twice.

Done.

Nit: I suppose it would work for any int-based type <=64 (char, int16_t, etc.)?

Probably. However, the types listed here are the exact types listed in nanopb's docs, so I'm reluctant to doc int16_t (etc) as working (assuming it does.)

[var-const]

Well, we're already hiding the implementation. Note that EncodeVarint is only called via the other Encode* methods such as EncodeNull, EncodeBool, EncodeLong (not yet present, but see next PR), etc. I suppose those methods could be replaced with a template function though... (possibly excepting EncodeNull).

Ok, it sounds like it's not worth the additional complexity, then.

For the record, I thought of something like:

template <typename T>
void EncodeIntegral(T value, /*...*/) {
  static_assert(std::is_integral<T>::value, "Value is not int-like");
  auto varint = static_cast<uint64_t>(value);
  // ...
}
// ...
EncodeIntegral(123); // OK
EncodeIntegral(-123); // OK
EncodeIntegral(true); // OK
EncodeIntegral(1.23); // static assertion failed: "Value is not int-like"
EncodeIntegral("foo"); // static assertion failed: "Value is not int-like"

EDIT: took a look at the follow-up PR. Yeah, I thought about skipping the EncodeBool/etc. functions and just having type tags right in the switch:

case FieldValue::Type::Long:
  EncodeIntegral(&stream, google_firestore_v1beta1_Value_null_value_tag, value.value.integer_value);
  break;

I guess it's also possible to put all those type->tag mappings into a tuple... Feel free to ignore.

[var-const]

All the same comments as its counterpart function.

[rsgowman]

Done.

[var-const]

Well, we're already hiding the implementation. Note that EncodeVarint is only called via the other Encode* methods such as EncodeNull, EncodeBool, EncodeLong (not yet present, but see next PR), etc. I suppose those methods could be replaced with a template function though... (possibly excepting EncodeNull).

Ok, it sounds like it's not worth the additional complexity, then.

For the record, I thought of something like:

template <typename T>
void EncodeIntegral(T value, /*...*/) {
  static_assert(std::is_integral<T>::value, "Value is not int-like");
  auto varint = static_cast<uint64_t>(value);
  // ...
}
// ...
EncodeIntegral(123); // OK
EncodeIntegral(-123); // OK
EncodeIntegral(true); // OK
EncodeIntegral(1.23); // static assertion failed: "Value is not int-like"
EncodeIntegral("foo"); // static assertion failed: "Value is not int-like"

EDIT: took a look at the follow-up PR. Yeah, I thought about skipping the EncodeBool/etc. functions and just having type tags right in the switch:

case FieldValue::Type::Long:
  EncodeIntegral(&stream, google_firestore_v1beta1_Value_null_value_tag, value.value.integer_value);
  break;

I guess it's also possible to put all those type->tag mappings into a tuple... Feel free to ignore.

[wilhuff]

LGTM