Skip to content

Fix #207: FR: Add CopyBytesFrom for array types #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
malay-satapathy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #207: FR: Add CopyBytesFrom for array types #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions runtime/cpp/emboss_array_view.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,29 @@ class GenericArrayView final {
WriteArrayToTextStream(this, stream, options);
}

template <class Container, int N = 0>
typename ::std::enable_if<
((void)N, kAddressableUnitSize * kElementSize == 8), void>::type
CopyBytesFrom(const Container &source) const {
EMBOSS_CHECK(IsComplete());
EMBOSS_CHECK(static_cast<::std::size_t>(source.size()) == ElementCount());
auto it = source.begin();
for (::std::size_t i = 0; i < ElementCount(); ++i, ++it) {
(*this)[i].Write(static_cast<typename ElementView::ValueType>(*it));
}
}
Comment on lines +261 to +271

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using operator[] on the source container restricts CopyBytesFrom to containers that support random access (like std::vector or std::array). Using iterators instead is more idiomatic in C++ templates and allows the method to support any container with begin() and end() (such as std::list or std::forward_list).\n\nAdditionally, to support braced initializer lists directly (e.g., CopyBytesFrom({0xaa, 0xbb})), you could consider adding an overload taking std::initializer_list<T> in the future.

  template <class Container, int N = 0>\n  typename ::std::enable_if<\n      ((void)N, kAddressableUnitSize * kElementSize == 8), void>::type\n  CopyBytesFrom(const Container &source) const {\n    EMBOSS_CHECK(Ok());\n    EMBOSS_CHECK(source.size() == ElementCount());\n    auto it = source.begin();\n    for (::std::size_t i = 0; i < ElementCount(); ++i, ++it) {\n      (*this)[i].Write(static_cast<typename ElementView::ValueType>(*it));\n    }\n  }

Comment on lines +261 to +271

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current implementation of CopyBytesFrom expects the source container to have .size() and .begin() member functions. While this works perfectly for standard containers like std::vector or std::array, it will fail to compile if a developer attempts to pass a raw C-style array (e.g., uint8_t buffer[4]), which is a very common use case in embedded and systems programming.\n\nConsider adding an overload of CopyBytesFrom specifically for raw arrays to improve usability and prevent compilation errors.

  template <class Container, int N = 0>\n  typename ::std::enable_if<\n      ((void)N, kAddressableUnitSize * kElementSize == 8), void>::type\n  CopyBytesFrom(const Container &source) const {\n    EMBOSS_CHECK(Ok());\n    EMBOSS_CHECK(source.size() == ElementCount());\n    auto it = source.begin();\n    for (::std::size_t i = 0; i < ElementCount(); ++i, ++it) {\n      (*this)[i].Write(static_cast<typename ElementView::ValueType>(*it));\n    }\n  }\n\n  template <typename T, ::std::size_t N, int M = 0>\n  typename ::std::enable_if<\n      ((void)M, kAddressableUnitSize * kElementSize == 8), void>::type\n  CopyBytesFrom(const T (&source)[N]) const {\n    EMBOSS_CHECK(Ok());\n    EMBOSS_CHECK(N == ElementCount());\n    for (::std::size_t i = 0; i < ElementCount(); ++i) {\n      (*this)[i].Write(static_cast<typename ElementView::ValueType>(source[i]));\n    }\n  }

Comment on lines +261 to +271

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Using EMBOSS_CHECK(Ok()) before writing values prevents using CopyBytesFrom to initialize or overwrite an array that is currently in an invalid state (e.g., when newly allocated with zero/garbage bytes that violate field constraints).

In Emboss, write operations (like Write or TryToWrite on individual views) only require the view to be complete (IsComplete()), not already valid (Ok()). We should only check EMBOSS_CHECK(IsComplete()) here to allow writing to currently-invalid arrays.

  template <class Container, int N = 0>
  typename ::std::enable_if<
      ((void)N, kAddressableUnitSize * kElementSize == 8), void>::type
  CopyBytesFrom(const Container &source) const {
    EMBOSS_CHECK(IsComplete());
    EMBOSS_CHECK(source.size() == ElementCount());
    auto it = source.begin();
    for (::std::size_t i = 0; i < ElementCount(); ++i, ++it) {
      (*this)[i].Write(static_cast<typename ElementView::ValueType>(*it));
    }
  }


template <typename T, ::std::size_t N, int M = 0>
typename ::std::enable_if<
((void)M, kAddressableUnitSize * kElementSize == 8), void>::type
CopyBytesFrom(const T (&source)[N]) const {
EMBOSS_CHECK(IsComplete());
EMBOSS_CHECK(N == ElementCount());
for (::std::size_t i = 0; i < ElementCount(); ++i) {
(*this)[i].Write(static_cast<typename ElementView::ValueType>(source[i]));
}
}
Comment on lines +273 to +282

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Similarly to the container overload, using EMBOSS_CHECK(Ok()) here prevents writing to an array that is currently in an invalid state. It should be changed to EMBOSS_CHECK(IsComplete()).

  template <typename T, ::std::size_t N, int M = 0>
  typename ::std::enable_if<
      ((void)M, kAddressableUnitSize * kElementSize == 8), void>::type
  CopyBytesFrom(const T (&source)[N]) const {
    EMBOSS_CHECK(IsComplete());
    EMBOSS_CHECK(N == ElementCount());
    for (::std::size_t i = 0; i < ElementCount(); ++i) {
      (*this)[i].Write(static_cast<typename ElementView::ValueType>(source[i]));
    }
  }


static constexpr bool IsAggregate() { return true; }

BufferType BackingStorage() const { return buffer_; }
Expand Down
38 changes: 38 additions & 0 deletions runtime/cpp/test/emboss_array_view_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <string>
#include <type_traits>
#include <array>

#include "absl/strings/str_format.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -89,6 +90,43 @@ TEST(ArrayView, Methods) {
EXPECT_FALSE((ArrayView<FixedUIntView<32>, ReadWriteContiguousBuffer, 1>{
ReadWriteContiguousBuffer{
nullptr}}.Ok()));

// Test CopyBytesFrom
::std::array<::std::uint8_t, 4> new_bytes = {0xaa, 0xbb, 0xcc, 0xdd};
auto dest_array = ArrayView<FixedUIntView<8>, ReadWriteContiguousBuffer, 1>{
ReadWriteContiguousBuffer{bytes, 4}};
dest_array.CopyBytesFrom(new_bytes);
EXPECT_EQ(0xaa, bytes[0]);
EXPECT_EQ(0xbb, bytes[1]);
EXPECT_EQ(0xcc, bytes[2]);
EXPECT_EQ(0xdd, bytes[3]);
Comment on lines +94 to +102

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

It is highly recommended to add a test case verifying that CopyBytesFrom aborts when the source container size does not match the destination array size (using EXPECT_DEATH when EMBOSS_CHECK_ABORTS is enabled).

Suggested change
// Test CopyBytesFrom
::std::array<::std::uint8_t, 4> new_bytes = {0xaa, 0xbb, 0xcc, 0xdd};
auto dest_array = ArrayView<FixedUIntView<8>, ReadWriteContiguousBuffer, 1>{
ReadWriteContiguousBuffer{bytes, 4}};
dest_array.CopyBytesFrom(new_bytes);
EXPECT_EQ(0xaa, bytes[0]);
EXPECT_EQ(0xbb, bytes[1]);
EXPECT_EQ(0xcc, bytes[2]);
EXPECT_EQ(0xdd, bytes[3]);
// Test CopyBytesFrom
::std::array<::std::uint8_t, 4> new_bytes = {0xaa, 0xbb, 0xcc, 0xdd};
auto dest_array = ArrayView<FixedUIntView<8>, ReadWriteContiguousBuffer, 1>{
ReadWriteContiguousBuffer{bytes, 4}};
dest_array.CopyBytesFrom(new_bytes);
EXPECT_EQ(0xaa, bytes[0]);
EXPECT_EQ(0xbb, bytes[1]);
EXPECT_EQ(0xcc, bytes[2]);
EXPECT_EQ(0xdd, bytes[3]);
#if EMBOSS_CHECK_ABORTS
::std::array<::std::uint8_t, 3> wrong_size_bytes = {0xaa, 0xbb, 0xcc};
EXPECT_DEATH(dest_array.CopyBytesFrom(wrong_size_bytes), "");
#endif // EMBOSS_CHECK_ABORTS


// Test CopyBytesFrom with signed elements to ensure values >= 128 do not abort
bytes[0] = 0;
bytes[1] = 0;
bytes[2] = 0;
bytes[3] = 0;
auto dest_signed_array =
ArrayView<FixedIntView<8>, ReadWriteContiguousBuffer, 1>{
ReadWriteContiguousBuffer{bytes, 4}};
dest_signed_array.CopyBytesFrom(new_bytes);
EXPECT_EQ(0xaa, bytes[0]);
EXPECT_EQ(0xbb, bytes[1]);
EXPECT_EQ(0xcc, bytes[2]);
EXPECT_EQ(0xdd, bytes[3]);

// Test CopyBytesFrom with a raw C-style array
const ::std::uint8_t raw_bytes[4] = {0x11, 0x22, 0x33, 0x44};
dest_array.CopyBytesFrom(raw_bytes);
EXPECT_EQ(0x11, bytes[0]);
EXPECT_EQ(0x22, bytes[1]);
EXPECT_EQ(0x33, bytes[2]);
EXPECT_EQ(0x44, bytes[3]);

#if EMBOSS_CHECK_ABORTS
::std::array<::std::uint8_t, 3> wrong_size_bytes = {0xaa, 0xbb, 0xcc};
EXPECT_DEATH(dest_array.CopyBytesFrom(wrong_size_bytes), "");
#endif // EMBOSS_CHECK_ABORTS
}
Comment on lines +99 to 130

@gemini-code-assist gemini-code-assist Bot Oct 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To ensure that CopyBytesFrom works correctly for signed 8-bit arrays (such as FixedIntView<8>) and does not abort when copying byte values >= 128 (which are negative in 2's complement), we should add a test case that specifically verifies this behavior.

  EXPECT_EQ(0xaa, bytes[0]);
  EXPECT_EQ(0xbb, bytes[1]);
  EXPECT_EQ(0xcc, bytes[2]);
  EXPECT_EQ(0xdd, bytes[3]);

  // Test CopyBytesFrom with signed elements to ensure values >= 128 do not abort
  bytes[0] = 0; bytes[1] = 0; bytes[2] = 0; bytes[3] = 0;
  auto dest_signed_array = ArrayView<FixedIntView<8>, ReadWriteContiguousBuffer, 1>{
      ReadWriteContiguousBuffer{bytes, 4}};
  dest_signed_array.CopyBytesFrom(new_bytes);
  EXPECT_EQ(0xaa, bytes[0]);
  EXPECT_EQ(0xbb, bytes[1]);
  EXPECT_EQ(0xcc, bytes[2]);
  EXPECT_EQ(0xdd, bytes[3]);
}


TEST(ArrayView, Ok) {
Expand Down
Loading