pw_protobuf: Use pw::InlineString for string fields
- Use pw::InlineString instead of pw::Vector<char> for string fields.
- Temporarily overload pw::string::Copy() for pw::InlineString<> to
simplify transitioning away from pw::Vector<char>.
Requires: pigweed-internal:31961
Change-Id: I1416f2d85471da4c174052a085cd896a0b1dc084
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/110111
Commit-Queue: Wyatt Hepler <hepler@google.com>
Reviewed-by: Armando Montanez <amontanez@google.com>
diff --git a/pw_protobuf/BUILD.bazel b/pw_protobuf/BUILD.bazel
index 3b287c7..b4dbf07 100644
--- a/pw_protobuf/BUILD.bazel
+++ b/pw_protobuf/BUILD.bazel
@@ -68,6 +68,7 @@
"//pw_status",
"//pw_stream",
"//pw_stream:interval_reader",
+ "//pw_string:string",
"//pw_varint",
"//pw_varint:stream",
],
diff --git a/pw_protobuf/BUILD.gn b/pw_protobuf/BUILD.gn
index e6ec3f8..a68ae74 100644
--- a/pw_protobuf/BUILD.gn
+++ b/pw_protobuf/BUILD.gn
@@ -79,6 +79,7 @@
"message.cc",
"stream_decoder.cc",
]
+ deps = [ "$dir_pw_string:string" ]
}
pw_source_set("bytes_utils") {
diff --git a/pw_protobuf/CMakeLists.txt b/pw_protobuf/CMakeLists.txt
index 9bdc38f..f8d1966 100644
--- a/pw_protobuf/CMakeLists.txt
+++ b/pw_protobuf/CMakeLists.txt
@@ -54,6 +54,8 @@
pw_stream.interval_reader
pw_varint
pw_varint.stream
+ PRIVATE_DEPS
+ pw_string.string
SOURCES
decoder.cc
encoder.cc
diff --git a/pw_protobuf/docs.rst b/pw_protobuf/docs.rst
index 3a816c0..b25c02e 100644
--- a/pw_protobuf/docs.rst
+++ b/pw_protobuf/docs.rst
@@ -789,10 +789,10 @@
pw::Vector<std::byte, 64> serial_number;
};
-* `string` fields are represented by ``pw::Vector`` when the ``max_size`` option
- is set for that field. Since the size is provided, the string is not
- automatically null-terminated. :ref:`module-pw_string` provides utility
- methods to copy string data into and out of this vector.
+* `string` fields are represented by a :cpp:type:`pw::InlineString` when the
+ ``max_size`` option is set for that field. The string can hold up to
+ ``max_size`` characters, and is always null terminated. The null terminator is
+ not counted in ``max_size``.
.. code::
@@ -807,7 +807,7 @@
.. code:: c++
struct Employee::Message {
- pw::Vector<char, 128> name;
+ pw::InlineString<128> name;
};
* Nested messages with a dependency cycle, repeated scalar fields without a
diff --git a/pw_protobuf/encoder.cc b/pw_protobuf/encoder.cc
index 54a4677..7c8491d 100644
--- a/pw_protobuf/encoder.cc
+++ b/pw_protobuf/encoder.cc
@@ -30,6 +30,7 @@
#include "pw_status/try.h"
#include "pw_stream/memory_stream.h"
#include "pw_stream/stream.h"
+#include "pw_string/string.h"
#include "pw_varint/varint.h"
namespace pw::protobuf {
@@ -537,17 +538,18 @@
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
- // WriteLengthDelimitedField() to output it to the stream.
+ // bytes or string field with a maximum size. Struct member is
+ // pw::Vector<std::byte> for bytes or pw::InlineString<> for string.
+ // Use the contents as a span and call WriteLengthDelimitedField() to
+ // output it to the stream.
PW_CHECK(field.elem_size() == sizeof(std::byte),
"Mismatched message field type and size");
- const auto* vector =
- reinterpret_cast<const pw::Vector<const std::byte>*>(
- values.data());
- if (!vector->empty()) {
- PW_TRY(WriteLengthDelimitedField(
- field.field_number(), span(vector->data(), vector->size())));
+ if (field.is_string()) {
+ PW_TRY(WriteStringOrBytes<const InlineString<>>(
+ field.field_number(), values.data()));
+ } else {
+ PW_TRY(WriteStringOrBytes<const Vector<const std::byte>>(
+ field.field_number(), values.data()));
}
}
break;
diff --git a/pw_protobuf/public/pw_protobuf/encoder.h b/pw_protobuf/public/pw_protobuf/encoder.h
index 7e8becb..ee3d52b 100644
--- a/pw_protobuf/public/pw_protobuf/encoder.h
+++ b/pw_protobuf/public/pw_protobuf/encoder.h
@@ -747,6 +747,16 @@
span<const std::byte> values,
size_t elem_size);
+ template <typename Container>
+ Status WriteStringOrBytes(uint32_t field_number,
+ const std::byte* raw_container) {
+ const auto& container = *reinterpret_cast<Container*>(raw_container);
+ if (container.empty()) {
+ return OkStatus();
+ }
+ return WriteLengthDelimitedField(field_number, as_bytes(span(container)));
+ }
+
// Checks if a write is invalid or will cause the encoder to enter an error
// state, and preemptively sets this encoder's status to that error to block
// the write. Only the first error encountered is tracked.
diff --git a/pw_protobuf/public/pw_protobuf/stream_decoder.h b/pw_protobuf/public/pw_protobuf/stream_decoder.h
index 6ca8803..edbb877 100644
--- a/pw_protobuf/public/pw_protobuf/stream_decoder.h
+++ b/pw_protobuf/public/pw_protobuf/stream_decoder.h
@@ -694,6 +694,18 @@
}
}
+ template <typename Container>
+ Status ReadStringOrBytesField(std::byte* raw_container) {
+ auto& container = *reinterpret_cast<Container*>(raw_container);
+ if (container.capacity() < delimited_field_size_) {
+ return Status::ResourceExhausted();
+ }
+ container.resize(container.capacity());
+ const auto sws = ReadDelimitedField(as_writable_bytes(span(container)));
+ container.resize(sws.size());
+ return sws.status();
+ }
+
Status CheckOkToRead(WireType type);
stream::Reader& reader_;
diff --git a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
index 55116b1..8e12cd5 100644
--- a/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
+++ b/pw_protobuf/py/pw_protobuf/codegen_pwpb.py
@@ -353,6 +353,15 @@
"""True if this field is a string field (as opposed to bytes)."""
return False
+ @staticmethod
+ def repeated_field_container(type_name: str, max_size: int) -> str:
+ """Returns the container type used for repeated fields.
+
+ Defaults to ::pw::Vector<type, max_size>. String fields use
+ ::pw::InlineString<max_size> instead.
+ """
+ return f'::pw::Vector<{type_name}, {max_size}>'
+
def use_callback(self) -> bool: # pylint: disable=no-self-use
"""Returns whether the decoder should use a callback."""
options = self._field.options()
@@ -411,7 +420,7 @@
max_size), self.name())
# Otherwise prefer pw::Vector for repeated fields.
- return ('::pw::Vector<{}, {}>'.format(self.type_name(from_root),
+ return (self.repeated_field_container(self.type_name(from_root),
max_size), self.name())
def _varint_type_table_entry(self) -> str:
@@ -1583,6 +1592,10 @@
def is_string(self) -> bool:
return True
+ @staticmethod
+ def repeated_field_container(type_name: str, max_size: int) -> str:
+ return f'::pw::InlineBasicString<{type_name}, {max_size}>'
+
def _size_fn(self) -> str:
# This uses the WithoutValue method to ensure that the maximum length
# of the delimited field size varint is used. This accounts for scratch
@@ -2265,6 +2278,7 @@
output.write_line('#include "pw_span/span.h"')
output.write_line('#include "pw_status/status.h"')
output.write_line('#include "pw_status/status_with_size.h"')
+ output.write_line('#include "pw_string/string.h"')
for imported_file in file_descriptor_proto.dependency:
generated_header = _proto_filename_to_generated_header(imported_file)
diff --git a/pw_protobuf/stream_decoder.cc b/pw_protobuf/stream_decoder.cc
index a781cf6..c94b8e8 100644
--- a/pw_protobuf/stream_decoder.cc
+++ b/pw_protobuf/stream_decoder.cc
@@ -32,6 +32,7 @@
#include "pw_status/status.h"
#include "pw_status/status_with_size.h"
#include "pw_status/try.h"
+#include "pw_string/string.h"
#include "pw_varint/stream.h"
#include "pw_varint/varint.h"
@@ -679,21 +680,15 @@
"Mismatched message field type and size");
PW_TRY(ReadDelimitedField(out));
} else {
- // bytes or string field with a maximum size. Struct member is a
- // pw::Vector<std::byte>. There's no vector equivalent of
- // ReadDelimitedField() to call, so just implement what that would
- // look like here (since it wouldn't be used anywhere else).
+ // bytes or string field with a maximum size. The struct member is
+ // pw::Vector<std::byte> for bytes or pw::InlineString<> for string.
PW_CHECK(field->elem_size() == sizeof(std::byte),
"Mismatched message field type and size");
- auto* vector = reinterpret_cast<pw::Vector<std::byte>*>(out.data());
- if (vector->capacity() < delimited_field_size_) {
- return Status::ResourceExhausted();
+ if (field->is_string()) {
+ PW_TRY(ReadStringOrBytesField<pw::InlineString<>>(out.data()));
+ } else {
+ PW_TRY(ReadStringOrBytesField<pw::Vector<std::byte>>(out.data()));
}
- vector->resize(vector->capacity());
- const auto sws =
- ReadDelimitedField(span(vector->data(), vector->size()));
- vector->resize(sws.size());
- PW_TRY(sws);
}
break;
}
diff --git a/pw_protobuf_compiler/proto.cmake b/pw_protobuf_compiler/proto.cmake
index 7783461..473a044 100644
--- a/pw_protobuf_compiler/proto.cmake
+++ b/pw_protobuf_compiler/proto.cmake
@@ -196,6 +196,7 @@
pw_build
pw_protobuf
pw_span
+ pw_string.string
${DEPS}
)
add_dependencies("${NAME}.pwpb" "${NAME}._generate.pwpb")
diff --git a/pw_protobuf_compiler/proto.gni b/pw_protobuf_compiler/proto.gni
index 41068dc..11cb828 100644
--- a/pw_protobuf_compiler/proto.gni
+++ b/pw_protobuf_compiler/proto.gni
@@ -199,6 +199,7 @@
deps = [ ":$target_name._gen($pw_protobuf_compiler_TOOLCHAIN)" ]
public_deps = [
"$dir_pw_containers:vector",
+ "$dir_pw_string:string",
dir_pw_assert,
dir_pw_function,
dir_pw_preprocessor,
diff --git a/pw_protobuf_compiler/pw_proto_library.bzl b/pw_protobuf_compiler/pw_proto_library.bzl
index 3cf3d9d..034ada5 100644
--- a/pw_protobuf_compiler/pw_proto_library.bzl
+++ b/pw_protobuf_compiler/pw_proto_library.bzl
@@ -309,6 +309,7 @@
"//pw_result",
"//pw_span",
"//pw_status",
+ "//pw_string:string",
],
"include_nanopb_dep": False,
"include_pwpb_dep": False,
diff --git a/pw_protobuf_compiler/pwpb_test.cc b/pw_protobuf_compiler/pwpb_test.cc
index 10c3962..c023697 100644
--- a/pw_protobuf_compiler/pwpb_test.cc
+++ b/pw_protobuf_compiler/pwpb_test.cc
@@ -20,5 +20,5 @@
EXPECT_EQ(point.x, 4u);
EXPECT_EQ(point.y, 8u);
EXPECT_EQ(point.name.size(), 5u);
- EXPECT_EQ(point.name.view(), "point");
+ EXPECT_EQ(point.name, "point");
}
diff --git a/pw_rpc/test_helpers_test.cc b/pw_rpc/test_helpers_test.cc
index d5d1e7e..335424e 100644
--- a/pw_rpc/test_helpers_test.cc
+++ b/pw_rpc/test_helpers_test.cc
@@ -70,7 +70,7 @@
return notifier_.try_acquire_for(duration);
}
- pw::Result<pw::Vector<char, 64>> LastEcho() const {
+ pw::Result<pw::InlineString<64>> LastEcho() const {
std::lock_guard<pw::sync::InterruptSpinLock> lock(lock_);
return last_echo_;
}
@@ -79,7 +79,7 @@
pw_rpc::pwpb::EchoService::Client& echo_client_;
PwpbUnaryReceiver<EchoMessage::Message> call_;
pw::sync::TimedThreadNotification notifier_;
- pw::Result<pw::Vector<char, 64>> last_echo_ PW_GUARDED_BY(lock_);
+ pw::Result<pw::InlineString<64>> last_echo_ PW_GUARDED_BY(lock_);
mutable pw::sync::InterruptSpinLock lock_;
};
@@ -108,9 +108,9 @@
// a separate thread we still need to wait with the timeout.
ASSERT_TRUE(entity.WaitForEcho(kWaitForEchoTimeout));
- pw::Result<pw::Vector<char, 64>> result = entity.LastEcho();
+ pw::Result<pw::InlineString<64>> result = entity.LastEcho();
ASSERT_TRUE(result.ok());
- EXPECT_EQ(result.value(), (pw::Vector<char, 64>{"Hello"}));
+ EXPECT_EQ(result.value(), "Hello");
}
TEST(RpcTestHelpersTest, SendResponseIfCalledNotOk) {
diff --git a/pw_software_update/bundled_update_service_pwpb.cc b/pw_software_update/bundled_update_service_pwpb.cc
index 29dbafe..0f26b07 100644
--- a/pw_software_update/bundled_update_service_pwpb.cc
+++ b/pw_software_update/bundled_update_service_pwpb.cc
@@ -98,7 +98,7 @@
// Enable bundle transfer.
Result<uint32_t> possible_transfer_id =
- backend_.EnableBundleTransferHandler(request.bundle_filename.view());
+ backend_.EnableBundleTransferHandler(request.bundle_filename);
if (!possible_transfer_id.ok()) {
SET_ERROR(BundledUpdateResult::Enum::kTransferFailed,
"Couldn't enable bundle transfer");
diff --git a/pw_string/BUILD.bazel b/pw_string/BUILD.bazel
index b2b5cc8..8ddf733 100644
--- a/pw_string/BUILD.bazel
+++ b/pw_string/BUILD.bazel
@@ -115,6 +115,7 @@
includes = ["public"],
deps = [
":pw_string",
+ ":string",
"//pw_containers:vector",
"//pw_status",
],
diff --git a/pw_string/BUILD.gn b/pw_string/BUILD.gn
index 55cebfa..c8b55c8 100644
--- a/pw_string/BUILD.gn
+++ b/pw_string/BUILD.gn
@@ -124,6 +124,7 @@
public = [ "public/pw_string/vector.h" ]
public_deps = [
":pw_string",
+ ":string",
"$dir_pw_containers:vector",
"$dir_pw_status",
dir_pw_span,
diff --git a/pw_string/public/pw_string/vector.h b/pw_string/public/pw_string/vector.h
index bd25b9472..e37e8a9 100644
--- a/pw_string/public/pw_string/vector.h
+++ b/pw_string/public/pw_string/vector.h
@@ -13,13 +13,14 @@
// the License.
#pragma once
+#include <algorithm>
#include <cstddef>
#include <string_view>
#include "pw_containers/vector.h"
-#include "pw_span/span.h"
#include "pw_status/status.h"
#include "pw_status/status_with_size.h"
+#include "pw_string/string.h"
#include "pw_string/util.h"
namespace pw {
@@ -48,11 +49,6 @@
copied);
}
-inline StatusWithSize Copy(const char* source, pw::Vector<char>& dest) {
- PW_DASSERT(source != nullptr);
- return Copy(ClampedCString(source, dest.capacity()), dest);
-}
-
inline StatusWithSize Copy(const pw::Vector<char>& source, span<char> dest) {
if (dest.empty()) {
return StatusWithSize::ResourceExhausted();
@@ -67,5 +63,30 @@
copied);
}
+inline StatusWithSize Copy(const char* source, pw::Vector<char>& dest) {
+ PW_DASSERT(source != nullptr);
+ return Copy(ClampedCString(source, dest.capacity()), dest);
+}
+
+// Overload for InlineString to simplify the transition from using Vector<char>
+// for protobuf string fields. An external Copy() function is not necessary for
+// InlineString; use assign() or the assignment operator instead.
+//
+// This function will be removed once projects switch to pw::InlineString<>.
+inline StatusWithSize Copy(const std::string_view& source,
+ pw::InlineString<>& dest) {
+ size_t copied = std::min(source.size(), static_cast<size_t>(dest.capacity()));
+ dest.assign(source, 0, copied);
+ return StatusWithSize(
+ copied == source.size() ? OkStatus() : Status::ResourceExhausted(),
+ copied);
+}
+
+inline StatusWithSize Copy(const char* source, pw::InlineString<>& dest) {
+ PW_DASSERT(source != nullptr);
+ // Clamp to capacity + 1 so strings larger than the capacity yield an error.
+ return Copy(ClampedCString(source, dest.capacity() + 1), dest);
+}
+
} // namespace string
} // namespace pw
