pw_protobuf: Return a Result from Encode()
This updates Encoder::Encode() to return a Result to make it less
awkward to use and to ensure that its status is checked by the caller.
Change-Id: Ie376ee07555199010e4363d013959bb24b0db79a
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/19620
Reviewed-by: Ewout van Bekkum <ewout@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Alexei Frolov <frolv@google.com>
diff --git a/pw_protobuf/BUILD.gn b/pw_protobuf/BUILD.gn
index b4638a7..c4ee3ed 100644
--- a/pw_protobuf/BUILD.gn
+++ b/pw_protobuf/BUILD.gn
@@ -28,8 +28,10 @@
pw_source_set("pw_protobuf") {
public_configs = [ ":default_config" ]
public_deps = [
- "$dir_pw_status",
- "$dir_pw_varint",
+ dir_pw_bytes,
+ dir_pw_result,
+ dir_pw_status,
+ dir_pw_varint,
]
public = [
"public/pw_protobuf/codegen.h",
diff --git a/pw_protobuf/codegen_test.cc b/pw_protobuf/codegen_test.cc
index 0b26657..72e11a7 100644
--- a/pw_protobuf/codegen_test.cc
+++ b/pw_protobuf/codegen_test.cc
@@ -166,10 +166,11 @@
};
// clang-format on
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -185,10 +186,11 @@
constexpr uint8_t expected_proto[] = {
0x08, 0x00, 0x08, 0x10, 0x08, 0x20, 0x08, 0x30};
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -201,10 +203,11 @@
repeated_test.WriteUint32s(values);
constexpr uint8_t expected_proto[] = {0x0a, 0x04, 0x00, 0x10, 0x20, 0x30};
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -221,10 +224,11 @@
constexpr uint8_t expected_proto[] = {
0x1a, 0x03, 't', 'h', 'e', 0x1a, 0x5, 'q', 'u', 'i', 'c', 'k',
0x1a, 0x5, 'b', 'r', 'o', 'w', 'n', 0x1a, 0x3, 'f', 'o', 'x'};
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -245,10 +249,11 @@
0x01, 0x10, 0x02, 0x2a, 0x04, 0x08, 0x02, 0x10, 0x04};
// clang-format on
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -269,10 +274,11 @@
constexpr uint8_t expected_proto[] = {
0x08, 0x03, 0x1a, 0x06, 0x0a, 0x04, 0xde, 0xad, 0xbe, 0xef};
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
- EXPECT_EQ(proto.size(), sizeof(expected_proto));
- EXPECT_EQ(std::memcmp(proto.data(), expected_proto, sizeof(expected_proto)),
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(expected_proto));
+ EXPECT_EQ(std::memcmp(
+ result.value().data(), expected_proto, sizeof(expected_proto)),
0);
}
@@ -293,8 +299,7 @@
end.WriteNanoseconds(490367432);
}
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
+ EXPECT_EQ(encoder.Encode().status(), Status::Ok());
}
TEST(Codegen, NonPigweedPackage) {
@@ -306,8 +311,7 @@
packed.WriteRep(std::span<const int64_t>(repeated));
packed.WritePacked("packed");
- std::span<const std::byte> proto;
- EXPECT_EQ(encoder.Encode(&proto), Status::Ok());
+ EXPECT_EQ(encoder.Encode().status(), Status::Ok());
}
} // namespace
diff --git a/pw_protobuf/encoder.cc b/pw_protobuf/encoder.cc
index 6a5eceb..66174fc 100644
--- a/pw_protobuf/encoder.cc
+++ b/pw_protobuf/encoder.cc
@@ -126,16 +126,14 @@
return Status::Ok();
}
-Status Encoder::Encode(std::span<const std::byte>* out) {
+Result<ConstByteSpan> Encoder::Encode() {
if (!encode_status_.ok()) {
- *out = std::span<const std::byte>();
return encode_status_;
}
if (blob_count_ == 0) {
// If there are no nested blobs, the buffer already contains a valid proto.
- *out = buffer_.first(EncodedSize());
- return Status::Ok();
+ return Result<ConstByteSpan>(buffer_.first(EncodedSize()));
}
union {
@@ -179,8 +177,7 @@
// Point the cursor to the end of the encoded proto.
cursor_ = write_cursor;
- *out = buffer_.first(EncodedSize());
- return Status::Ok();
+ return Result<ConstByteSpan>(buffer_.first(EncodedSize()));
}
} // namespace pw::protobuf
diff --git a/pw_protobuf/encoder_fuzzer.cc b/pw_protobuf/encoder_fuzzer.cc
index e0991d0..1ede047 100644
--- a/pw_protobuf/encoder_fuzzer.cc
+++ b/pw_protobuf/encoder_fuzzer.cc
@@ -133,7 +133,6 @@
ASAN_POISON_MEMORY_REGION(poisoned, poisoned_length);
pw::protobuf::NestedEncoder encoder(unpoisoned);
- std::span<const std::byte> out;
// Storage for generated spans
std::vector<uint32_t> u32s;
@@ -154,7 +153,7 @@
switch (provider.ConsumeEnum<FieldType>()) {
case kEncodeAndClear:
// Special "field". Encode all the fields so far and reset the encoder.
- encoder.Encode(&out);
+ encoder.Encode();
encoder.Clear();
break;
case kUint32:
@@ -278,7 +277,7 @@
}
}
// Ensure we call `Encode` at least once.
- encoder.Encode(&out);
+ encoder.Encode();
// Don't forget to unpoison for the next iteration!
ASAN_UNPOISON_MEMORY_REGION(poisoned, poisoned_length);
diff --git a/pw_protobuf/encoder_test.cc b/pw_protobuf/encoder_test.cc
index 109943d..605467b 100644
--- a/pw_protobuf/encoder_test.cc
+++ b/pw_protobuf/encoder_test.cc
@@ -93,10 +93,12 @@
EXPECT_EQ(encoder.WriteString(kTestProtoErrorMessageField, "broken 💩"),
Status::Ok());
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
TEST(Encoder, EncodeInsufficientSpace) {
@@ -115,9 +117,7 @@
EXPECT_EQ(encoder.WriteFloat(kTestProtoRatioField, 1.618034),
Status::ResourceExhausted());
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::ResourceExhausted());
- EXPECT_EQ(encoded.size(), 0u);
+ ASSERT_EQ(encoder.Encode().status(), Status::ResourceExhausted());
}
TEST(Encoder, EncodeInvalidArguments) {
@@ -133,9 +133,7 @@
encoder.Clear();
EXPECT_EQ(encoder.WriteBool(19091, false), Status::InvalidArgument());
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::InvalidArgument());
- EXPECT_EQ(encoded.size(), 0u);
+ ASSERT_EQ(encoder.Encode().status(), Status::InvalidArgument());
}
TEST(Encoder, Nested) {
@@ -214,10 +212,12 @@
};
// clang-format on
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
TEST(Encoder, NestedDepthLimit) {
@@ -274,10 +274,12 @@
constexpr uint8_t encoded_proto[] = {
0x08, 0x00, 0x08, 0x32, 0x08, 0x64, 0x08, 0x96, 0x01, 0x08, 0xc8, 0x01};
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
TEST(Encoder, PackedVarint) {
@@ -292,10 +294,12 @@
0x0a, 0x07, 0x00, 0x32, 0x64, 0x96, 0x01, 0xc8, 0x01};
// key size v[0] v[1] v[2] v[3] v[4]
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
TEST(Encoder, PackedVarintInsufficientSpace) {
@@ -305,9 +309,7 @@
constexpr uint32_t values[] = {0, 50, 100, 150, 200};
encoder.WritePackedUint32(1, values);
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::ResourceExhausted());
- EXPECT_EQ(encoded.size(), 0u);
+ EXPECT_EQ(encoder.Encode().status(), Status::ResourceExhausted());
}
TEST(Encoder, PackedFixed) {
@@ -327,10 +329,12 @@
0x00, 0x00, 0x00, 0x96, 0x00, 0x00, 0x00, 0xc8, 0x00, 0x00, 0x00,
0x12, 0x08, 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01};
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
TEST(Encoder, PackedZigzag) {
@@ -344,10 +348,12 @@
constexpr uint8_t encoded_proto[] = {
0x0a, 0x09, 0xc7, 0x01, 0x31, 0x01, 0x00, 0x02, 0x32, 0xc8, 0x01};
- std::span<const std::byte> encoded;
- EXPECT_EQ(encoder.Encode(&encoded), Status::Ok());
- EXPECT_EQ(encoded.size(), sizeof(encoded_proto));
- EXPECT_EQ(std::memcmp(encoded.data(), encoded_proto, encoded.size()), 0);
+ Result result = encoder.Encode();
+ ASSERT_EQ(result.status(), Status::Ok());
+ EXPECT_EQ(result.value().size(), sizeof(encoded_proto));
+ EXPECT_EQ(
+ std::memcmp(result.value().data(), encoded_proto, sizeof(encoded_proto)),
+ 0);
}
} // namespace
diff --git a/pw_protobuf/public/pw_protobuf/encoder.h b/pw_protobuf/public/pw_protobuf/encoder.h
index 14483d2..c859009 100644
--- a/pw_protobuf/public/pw_protobuf/encoder.h
+++ b/pw_protobuf/public/pw_protobuf/encoder.h
@@ -17,7 +17,9 @@
#include <cstring>
#include <span>
+#include "pw_bytes/span.h"
#include "pw_protobuf/wire_format.h"
+#include "pw_result/result.h"
#include "pw_status/status.h"
#include "pw_varint/varint.h"
@@ -31,7 +33,7 @@
// message. This can be templated to minimize the overhead.
using SizeType = size_t;
- constexpr Encoder(std::span<std::byte> buffer,
+ constexpr Encoder(ByteSpan buffer,
std::span<SizeType*> locations,
std::span<SizeType*> stack)
: buffer_(buffer),
@@ -225,7 +227,7 @@
}
// Writes a proto bytes key-value pair.
- Status WriteBytes(uint32_t field_number, std::span<const std::byte> value) {
+ Status WriteBytes(uint32_t field_number, ConstByteSpan value) {
std::byte* original_cursor = cursor_;
WriteFieldKey(field_number, WireType::kDelimited);
WriteVarint(value.size_bytes());
@@ -266,7 +268,18 @@
// Runs a final encoding pass over the intermediary data and returns the
// encoded protobuf message.
- Status Encode(std::span<const std::byte>* out);
+ Result<ConstByteSpan> Encode();
+
+ // DEPRECATED. Use Encode() instead.
+ // TODO(frolv): Remove this after all references to it are updated.
+ Status Encode(ConstByteSpan* out) {
+ Result result = Encode();
+ if (!result.ok()) {
+ return result.status();
+ }
+ *out = result.value();
+ return Status::Ok();
+ }
private:
constexpr bool ValidFieldNumber(uint32_t field_number) const {
@@ -340,7 +353,7 @@
}
// The buffer into which the proto is encoded.
- std::span<std::byte> buffer_;
+ ByteSpan buffer_;
std::byte* cursor_;
// List of pointers to sub-messages' delimiting size fields.
@@ -359,8 +372,7 @@
template <size_t kMaxNestedDepth = 1, size_t kMaxBlobs = 1>
class NestedEncoder : public Encoder {
public:
- NestedEncoder(std::span<std::byte> buffer)
- : Encoder(buffer, blobs_, stack_) {}
+ NestedEncoder(ByteSpan buffer) : Encoder(buffer, blobs_, stack_) {}
// Disallow copy/assign to avoid confusion about who owns the buffer.
NestedEncoder(const NestedEncoder& other) = delete;
diff --git a/pw_rpc/packet.cc b/pw_rpc/packet.cc
index 56969ca..d99cd84 100644
--- a/pw_rpc/packet.cc
+++ b/pw_rpc/packet.cc
@@ -80,12 +80,12 @@
rpc_packet.WriteMethodId(method_id_);
rpc_packet.WriteStatus(status_);
- std::span<const byte> proto;
- if (Status status = encoder.Encode(&proto); !status.ok()) {
- return StatusWithSize(status, 0);
+ Result result = encoder.Encode();
+ if (!result.ok()) {
+ return StatusWithSize(result.status(), 0);
}
- return StatusWithSize(proto.size());
+ return StatusWithSize(result.value().size());
}
size_t Packet::MinEncodedSizeBytes() const {
