Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Support empty strings and vectors in standard codec #12974

Merged
franciscojma86 merged 4 commits into from
Oct 15, 2019
Merged

Support empty strings and vectors in standard codec #12974

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f174c9
Support empty strings and vectors in standard codec
rokob Oct 7, 2019
a0e69c3
fix typo in test name
rokob Oct 7, 2019
25c0f0f
remove unnecessary length check in ReadValue for List
rokob Oct 7, 2019
ed41ed6
we also don't need this check before calling read as memcpy can handl…
rokob Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions shell/platform/common/cpp/client_wrapper/standard_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,10 @@ EncodableValue StandardCodecSerializer::ReadValue(
case EncodedType::kString: {
size_t size = ReadSize(stream);
std::string string_value;
string_value.resize(size);
stream->ReadBytes(reinterpret_cast<uint8_t*>(&string_value[0]), size);
if (size > 0) {
string_value.resize(size);
stream->ReadBytes(reinterpret_cast<uint8_t*>(&string_value[0]), size);
}
return EncodableValue(string_value);
}
case EncodedType::kUInt8List:
Expand All @@ -126,6 +128,9 @@ EncodableValue StandardCodecSerializer::ReadValue(
case EncodedType::kList: {
size_t length = ReadSize(stream);
EncodableList list_value;
if (length == 0) {
return EncodableValue(list_value);
}
list_value.reserve(length);
Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this causing a failure too? I think reserve(0) is just a no-op, and the for loop would be no-op as well.

Copy link
Contributor Author

@rokob rokob Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I was just being over cautious.

for (size_t i = 0; i < length; ++i) {
list_value.push_back(ReadValue(stream));
Expand Down Expand Up @@ -176,8 +181,10 @@ void StandardCodecSerializer::WriteValue(const EncodableValue& value,
const auto& string_value = value.StringValue();
size_t size = string_value.size();
WriteSize(size, stream);
Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be included in the if statement below?

Copy link
Contributor Author

@rokob rokob Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we always want to write the size because the format specifies type|size|contents and the reading code needs to read that zero to be able to decode an empty string.

Copy link
Contributor

@franciscojma86 franciscojma86 Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, thanks!

stream->WriteBytes(reinterpret_cast<const uint8_t*>(string_value.data()),
size);
if (size > 0) {
stream->WriteBytes(
reinterpret_cast<const uint8_t*>(string_value.data()), size);
}
break;
}
case EncodableValue::Type::kByteList:
Expand Down Expand Up @@ -259,6 +266,9 @@ void StandardCodecSerializer::WriteVector(
ByteBufferStreamWriter* stream) const {
size_t count = vector.size();
WriteSize(count, stream);
if (count == 0) {
return;
}
uint8_t type_size = static_cast<uint8_t>(sizeof(T));
if (type_size > 1) {
stream->WriteAlignment(type_size);
Expand Down
10 changes: 10 additions & 0 deletions shell/platform/common/cpp/client_wrapper/standard_message_codec_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ TEST(StandardMessageCodec, CanEncodeAndDecodeStringWithNonBMPCodePoint) {
CheckEncodeDecode(EncodableValue(u8"h\U0001F602w"), bytes);
}

TEST(StandardMessageCodec, CanEncodeAndDecodeEmptyString) {
std::vector<uint8_t> bytes = {0x07, 0x00};
CheckEncodeDecode(EncodableValue(u8""), bytes);
}

TEST(StandardMessageCodec, CanEncodeAndDecodeList) {
std::vector<uint8_t> bytes = {
0x0c, 0x05, 0x00, 0x07, 0x05, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x06,
Expand All @@ -117,6 +122,11 @@ TEST(StandardMessageCodec, CanEncodeAndDecodeList) {
CheckEncodeDecode(value, bytes);
}

TEST(StandardMessageCodec, CanEncodeAndDecodeEmptyList) {
std::vector<uint8_t> bytes = {0x0c, 0x00};
CheckEncodeDecode(EncodableValue(EncodableList{}), bytes);
}

TEST(StandardMessageCodec, CanEncodeAndDecodeMap) {
std::vector<uint8_t> bytes_prefix = {0x0d, 0x04};
EncodableValue value(EncodableMap{
Expand Down