pw_protobuf: Don't write empty fixed sized fields
Fixed sized fields are an implementation detail of pw_protobuf, and
should behave identically on-wire to those implemented with pw::Vector.
This means not encoding when they have a zero (default) value, for
compatibility with other protobuf implementations.
Confirmation that this is correct behavior is that after this, tests
for certain fields no longer need to include expected zero values
of random other fields.
Change-Id: I7f69d0fd1385c80000bbd28dc992123517e171d3
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/97901
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Pigweed-Auto-Submit: Scott James Remnant <keybuk@google.com>
Reviewed-by: Armando Montanez <amontanez@google.com>
diff --git a/pw_protobuf/codegen_message_test.cc b/pw_protobuf/codegen_message_test.cc
index de6111a..bc2bee3 100644
--- a/pw_protobuf/codegen_message_test.cc
+++ b/pw_protobuf/codegen_message_test.cc
@@ -1284,17 +1284,9 @@
const auto status = pigweed.Write(message);
ASSERT_EQ(status, OkStatus());
- // clang-format off
- constexpr uint8_t expected_proto[] = {
- // pigweed.bytes (default)
- 0x5a, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- };
- // clang-format on
-
+ // Since all fields are at their default, the output should be zero sized.
ConstByteSpan result = writer.WrittenData();
- EXPECT_EQ(result.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(result.data(), expected_proto, sizeof(expected_proto)),
- 0);
+ EXPECT_EQ(result.size(), 0u);
}
TEST(CodegenMessage, WritePackedScalar) {
@@ -1320,18 +1312,12 @@
0x10,
0x20,
0x30,
- // doubles[], v={0, 0} (default)
- 0x22, 0x10,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// fixed32s[]. v={0, 16, 32, 48}
0x32, 0x10,
0x00, 0x00, 0x00, 0x00,
0x10, 0x00, 0x00, 0x00,
0x20, 0x00, 0x00, 0x00,
0x30, 0x00, 0x00, 0x00,
- // uint64s[]. v={0, 0, 0, 0} (default)
- 0x42, 0x04, 0x00, 0x00, 0x00, 0x00
};
// clang-format on
@@ -1399,13 +1385,6 @@
0x00,
0x02,
0x32,
- // doubles[], v={0, 0} (default)
- 0x22, 0x10,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- // uint64s[]. v={0, 0, 0, 0} (default)
- 0x42, 0x04, 0x00, 0x00, 0x00, 0x00
-
};
// clang-format on
@@ -1432,12 +1411,6 @@
// clang-format off
constexpr uint8_t expected_proto[] = {
- // doubles[], v={0, 0} (default)
- 0x22, 0x10,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- // uint64s[]. v={0, 0, 0, 0} (default)
- 0x42, 0x04, 0x00, 0x00, 0x00, 0x00,
// enums[], v={RED, GREEN, AMBER, RED}
0x4a, 0x04, 0x00, 0x02, 0x01, 0x00,
};
@@ -1470,8 +1443,6 @@
// clang-format off
constexpr uint8_t expected_proto[] = {
- // pigweed.bytes (default)
- 0x5a, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// pigweed.description
0x62, 0x5c, 'a', 'n', ' ', 'o', 'p', 'e', 'n', ' ', 's', 'o', 'u', 'r', 'c',
'e', ' ', 'c', 'o', 'l', 'l', 'e', 'c', 't', 'i', 'o', 'n', ' ', 'o', 'f',
@@ -1508,8 +1479,6 @@
// clang-format off
constexpr uint8_t expected_proto[] = {
- // pigweed.bytes (default)
- 0x5a, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// pigweed.special_property
0x68, 0x2a,
};
@@ -1583,10 +1552,6 @@
// clang-format off
constexpr uint8_t expected_proto[] = {
- // doubles[], v={0, 0} (default)
- 0x22, 0x10,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
// repeated.structs
0x2a, 0x04,
// repeated.structs.one v=16
@@ -1599,8 +1564,6 @@
0x08, 0x30,
// repeated.structs.two v=64
0x10, 0x40,
- // uint64s[]. v={0, 0, 0, 0} (default)
- 0x42, 0x04, 0x00, 0x00, 0x00, 0x00
};
// clang-format on
@@ -1669,8 +1632,6 @@
0x0a, 0x04, 'c', 'h', 'i', 'p',
// pigweed.device_info.attributes[1].value
0x12, 0x08, 'l', 'e', 'f', 't', '-', 's', 'o', 'c',
- // pigweed.bytes (default)
- 0x5a, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
};
// clang-format on
diff --git a/pw_protobuf/encoder.cc b/pw_protobuf/encoder.cc
index b367766..15d6f6b 100644
--- a/pw_protobuf/encoder.cc
+++ b/pw_protobuf/encoder.cc
@@ -273,8 +273,12 @@
"Mismatched message field type and size");
if (field.is_fixed_size()) {
PW_CHECK(field.is_repeated(), "Non-repeated fixed size field");
- PW_TRY(WritePackedFixed(
- field.field_number(), values, field.elem_size()));
+ if (static_cast<size_t>(
+ std::count(values.begin(), values.end(), std::byte{0})) <
+ values.size()) {
+ PW_TRY(WritePackedFixed(
+ field.field_number(), values, field.elem_size()));
+ }
} else if (field.is_repeated()) {
// The struct member for this field is a vector of a type
// corresponding to the field element size. Cast to the correct
@@ -348,6 +352,11 @@
// the array so we're not performing type aliasing (except for
// unsigned vs signed which is explicitly allowed).
PW_CHECK(field.is_repeated(), "Non-repeated fixed size field");
+ if (static_cast<size_t>(
+ std::count(values.begin(), values.end(), std::byte{0})) ==
+ values.size()) {
+ continue;
+ }
if (field.elem_size() == sizeof(uint64_t)) {
PW_TRY(WritePackedVarints(
field.field_number(),
@@ -524,7 +533,11 @@
// Call WriteLengthDelimitedField() to output it to the stream.
PW_CHECK(field.elem_size() == sizeof(std::byte),
"Mismatched message field type and size");
- PW_TRY(WriteLengthDelimitedField(field.field_number(), values));
+ if (static_cast<size_t>(
+ std::count(values.begin(), values.end(), std::byte{0})) <
+ values.size()) {
+ PW_TRY(WriteLengthDelimitedField(field.field_number(), values));
+ }
} else {
// bytes or string field with a maximum size. Struct member is a
// pw::Vector<std::byte>. Use the contents as a span and call
