Tests for GenericAppendOnlyBuffer move operations added. #37667
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with |
gd-mauri
force-pushed
the
increase_unit_test_coverage_for_app_data_model_provider
branch
2 times, most recently
from
28f856b to
ff5d09d
Compare
|
PR #37667: Size comparison from 8464e5f to ff5d09d Full report (64 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
gd-mauri
force-pushed
the
increase_unit_test_coverage_for_app_data_model_provider
branch
from
ff5d09d to
92c1e30
Compare
|
PR #37667: Size comparison from 7dccded to 92c1e30 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
gd-mauri
force-pushed
the
increase_unit_test_coverage_for_app_data_model_provider
branch
from
92c1e30 to
b909826
Compare
gd-mauri
force-pushed
the
increase_unit_test_coverage_for_app_data_model_provider
branch
from
b909826 to
589f7f3
Compare
|
PR #37667: Size comparison from 5398152 to 589f7f3 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| list.Append(12); | ||
|
|
||
| // Get a ListBuilder base class object | ||
| GenericAppendOnlyBuffer originalBuffer{ static_cast<GenericAppendOnlyBuffer &&>(std::move(list)) }; |
There was a problem hiding this comment.
The static cast should not be needed, as it is what std::move does.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Comment on lines
+229
to
+230
| ASSERT_EQ(originalBuffer.Size(), size_t{ 0 }); // NOLINT(bugprone-use-after-move) | ||
| ASSERT_TRUE(originalBuffer.IsEmpty()); // NOLINT(bugprone-use-after-move) |
There was a problem hiding this comment.
This seems like a conceited test that does something we should not expect.
There was a problem hiding this comment.
here I was following @andreilitvin suggestion to assert original buffer (the one instance being moved from) gets its (observable) internal state cleared.
There was a problem hiding this comment.
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.
| list2.Append(21); | ||
|
|
||
| // Get a ListBuilder base class object | ||
| GenericAppendOnlyBuffer originalBuffer{ static_cast<GenericAppendOnlyBuffer &&>(std::move(list1)) }; |
There was a problem hiding this comment.
Remove all static cast to &&
There was a problem hiding this comment.
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.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The code changes introduce a new test case for GenericAppendOnlyBuffer move operations. The implementation is generally well-done, but I've left a few suggestions for improvement.
Summary of Findings
- Clarity of comments: The comment "move constructor called for the second time here" could be more precise. It's not immediately clear why it's the 'second time'. Consider rephrasing for better understanding.
- Consistency in naming: Consider renaming
list1andlist2tolistandanotherListrespectively, for consistency with the first test case and improved readability.
Assessment
The pull request introduces a new test case for GenericAppendOnlyBuffer move operations, which is a valuable addition to ensure the correctness of the data model provider. The implementation appears to be well-structured and covers both move construction and move assignment scenarios. However, there are a few minor points that could be improved for clarity and consistency. I would recommend addressing these comments before requesting a review from someone else, but feel free to request another review from Gemini via /gemini review when you have addressed these comments and I'll take another look! Note that I am not authorized to directly approve the pull request, and users should have others review and approve this code before merging.
|
@andy31415, @tcarmelveilleux please review this other one instead: closing this one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #37223
Testing
new
TEST_F(TestMetadataList, BufferMoveOperationsWork)added intosrc/app/data-model-provider/tests/TestMetadataList.cpp