Skip to content

Commit

Permalink
pw_protobuf: Add StreamEncoder::CloseEncoder
Browse files Browse the repository at this point in the history
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 <frolv@google.com>
Commit-Queue: Taylor Cramer <cramertj@google.com>
  • Loading branch information
cramertj authored and CQ Bot Account committed Jul 29, 2022
1 parent c8bb7b1 commit 07a61cc
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 2 deletions.
2 changes: 1 addition & 1 deletion pw_protobuf/encoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
13 changes: 13 additions & 0 deletions pw_protobuf/encoder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
15 changes: 14 additions & 1 deletion pw_protobuf/public/pw_protobuf/encoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
//
Expand Down

0 comments on commit 07a61cc

Please sign in to comment.