Skip to content
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

Tests for GenericAppendOnlyBuffer move operations added. #37667

Closed
wants to merge 2 commits into from
Closed

Tests for GenericAppendOnlyBuffer move operations added. #37667

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f765d7
Tests for GenericAppendOnlyBuffer move operations added.
gd-mauri Feb 19, 2025
589f7f3
fix for clang-tidy use after move error.
gd-mauri Feb 20, 2025
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
62 changes: 62 additions & 0 deletions src/app/data-model-provider/tests/TestMetadataList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

using namespace chip;
using namespace chip::app::DataModel;
using namespace chip::app::DataModel::detail;

namespace {

Expand Down Expand Up @@ -204,4 +205,65 @@ TEST_F(TestMetadataList, ListBuilderConvertersWorks)
}
}

TEST_F(TestMetadataList, BufferMoveOperationsWork)
{

{
ListBuilder<int> list;

ASSERT_EQ(list.EnsureAppendCapacity(3), CHIP_NO_ERROR);

list.Append(10);
list.Append(11);
list.Append(12);

// Get a ListBuilder base class object
GenericAppendOnlyBuffer originalBuffer{ static_cast<GenericAppendOnlyBuffer &&>(std::move(list)) };
Copy link
Contributor

Choose a reason for hiding this comment

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

The static cast should not be needed, as it is what std::move does.

Copy link
Contributor Author

@gd-mauri gd-mauri Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can try removing it. But list's type is not GenericAppendOnlyBuffer but ListBuilder<int> which in turn derives from GenericAppendOnlyBuffer .

if I'm not mistaken, std::move(list) will cast a ListBuilder<int> & into a ListBuilder<int> && , and then I still need to go from ListBuilder<int> && to GenericAppendOnlyBuffer &&, and that's why static_cast is used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should have listbuilder support the move constructor and that would in turn move the append-only. If listbuilder has no move suport, that sounds like a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please disregard this PR in favor of this new one: #37812

I had to recreate my environment so it was much easier to create a new branch with the existing commits.


ASSERT_EQ(originalBuffer.Size(), size_t{ 3 });
ASSERT_FALSE(originalBuffer.IsEmpty());

/// move constructor called for the second time here
GenericAppendOnlyBuffer newBuffer{ std::move(originalBuffer) };

ASSERT_EQ(originalBuffer.Size(), size_t{ 0 }); // NOLINT(bugprone-use-after-move)
ASSERT_TRUE(originalBuffer.IsEmpty()); // NOLINT(bugprone-use-after-move)
Comment on lines +229 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a conceited test that does something we should not expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here I was following @andreilitvin suggestion to assert original buffer (the one instance being moved from) gets its (observable) internal state cleared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please disregard this PR in favor of this new one: #37812

I had to recreate my environment so it was much easier to create a new branch with the existing commits.


ASSERT_EQ(newBuffer.Size(), size_t{ 3 });
ASSERT_FALSE(newBuffer.IsEmpty());
}

{
ListBuilder<int> list1;

ASSERT_EQ(list1.EnsureAppendCapacity(3), CHIP_NO_ERROR);

list1.Append(10);
list1.Append(11);
list1.Append(12);

ListBuilder<int> list2;

ASSERT_EQ(list2.EnsureAppendCapacity(2), CHIP_NO_ERROR);

list2.Append(20);
list2.Append(21);

// Get a ListBuilder base class object
GenericAppendOnlyBuffer originalBuffer{ static_cast<GenericAppendOnlyBuffer &&>(std::move(list1)) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all static cast to &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please disregard this PR in favor of this new one: #37812

I had to recreate my environment so it was much easier to create a new branch with the existing commits.


// Get another ListBuilder base class object
GenericAppendOnlyBuffer anotherBuffer{ static_cast<GenericAppendOnlyBuffer &&>(std::move(list2)) };

ASSERT_EQ(originalBuffer.Size(), size_t{ 3 });
ASSERT_EQ(anotherBuffer.Size(), size_t{ 2 });

// move assignemnt operator called here
originalBuffer = std::move(anotherBuffer);

ASSERT_EQ(originalBuffer.Size(), size_t{ 2 });
ASSERT_EQ(anotherBuffer.Size(), size_t{ 0 }); // NOLINT(bugprone-use-after-move)
ASSERT_TRUE(anotherBuffer.IsEmpty()); // NOLINT(bugprone-use-after-move)
}
}
} // namespace
Loading