From 07a61ccf0226a5a370bf93a3f275e561f07beb44 Mon Sep 17 00:00:00 2001 From: Taylor Cramer Date: Fri, 29 Jul 2022 22:55:40 +0000 Subject: [PATCH] pw_protobuf: Add StreamEncoder::CloseEncoder Previously, `StreamEncoder` would only perform writes to its parent after destruction occurred. This resulted in callers frequently needing to introduce new scopes, often with a comment explaining the API to readers. This change adds the `CloseEncoder` method, which explicitly finalizes the child encoder and performs the writes to the parent, allowing users both to close an encoder before the end of its lexical scope and to make their code more explicit and self-descriptive if desired. Change-Id: Ieedf99048da3fe2d87602eea1ae85165c378d70a Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/103922 Reviewed-by: Alexei Frolov Commit-Queue: Taylor Cramer --- pw_protobuf/encoder.cc | 2 +- pw_protobuf/encoder_test.cc | 13 +++++++++++++ pw_protobuf/public/pw_protobuf/encoder.h | 15 ++++++++++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pw_protobuf/encoder.cc b/pw_protobuf/encoder.cc index b7ddba3e8f..c7209f2b6c 100644 --- a/pw_protobuf/encoder.cc +++ b/pw_protobuf/encoder.cc @@ -65,7 +65,7 @@ StreamEncoder StreamEncoder::GetNestedEncoder(uint32_t field_number, return StreamEncoder(*this, nested_buffer, write_when_empty); } -StreamEncoder::~StreamEncoder() { +void StreamEncoder::CloseEncoder() { // If this was an invalidated StreamEncoder which cannot be used, permit the // object to be cleanly destructed by doing nothing. if (nested_field_number_ == kFirstReservedNumber) { diff --git a/pw_protobuf/encoder_test.cc b/pw_protobuf/encoder_test.cc index c4700a6356..48338141e5 100644 --- a/pw_protobuf/encoder_test.cc +++ b/pw_protobuf/encoder_test.cc @@ -502,5 +502,18 @@ TEST(StreamEncoder, NestedStatusPropagates) { ASSERT_EQ(parent.status(), Status::InvalidArgument()); } +TEST(StreamEncoder, ManualCloseEncoderWrites) { + std::byte encode_buffer[32]; + MemoryEncoder parent(encode_buffer); + StreamEncoder child = parent.GetNestedEncoder(kTestProtoNestedField); + child.CloseEncoder(); + ASSERT_EQ(parent.status(), OkStatus()); + const size_t kExpectedSize = + varint::EncodedSize( + FieldKey(kTestProtoNestedField, WireType::kDelimited)) + + varint::EncodedSize(0); + ASSERT_EQ(parent.size(), kExpectedSize); +} + } // namespace } // namespace pw::protobuf diff --git a/pw_protobuf/public/pw_protobuf/encoder.h b/pw_protobuf/public/pw_protobuf/encoder.h index 7582a81bab..2f8701fce4 100644 --- a/pw_protobuf/public/pw_protobuf/encoder.h +++ b/pw_protobuf/public/pw_protobuf/encoder.h @@ -128,7 +128,7 @@ class StreamEncoder { // // Postcondition: If this encoder is a nested one, the parent encoder is // unlocked and proto encoding may resume on the parent. - ~StreamEncoder(); + ~StreamEncoder() { CloseEncoder(); } // Disallow copy/assign to avoid confusion about who owns the buffer. StreamEncoder& operator=(const StreamEncoder& other) = delete; @@ -138,6 +138,19 @@ class StreamEncoder { // parent_ pointer to become invalid. StreamEncoder& operator=(StreamEncoder&& other) = delete; + // Closes this encoder, finalizing its output. + // + // This method is called automatically by `StreamEncoder`'s destructor, but + // may be invoked manually in order to close an encoder before the end of its + // lexical scope. + // + // Precondition: Encoder has no active child encoder. + // + // Postcondition: If this encoder is a nested one, the parent encoder is + // unlocked and proto encoding may resume on the parent. No more writes + // to this encoder may be performed. + void CloseEncoder(); + // Forwards the conservative write limit of the underlying // pw::stream::Writer. //