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.

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

iox-#2052 Implement move/copy constructor and assignment for FixedPositionContainer #2069

Conversation

Dennis40816
Copy link
Contributor

@Dennis40816 Dennis40816 commented Oct 29, 2023

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

  • Move/copy constructors and assignment for FixedPositionContainer are implemented.
  • At present, when invoking the move/copy constructor or assignment for FixedPositionContainer, only the move/copy assignment of elements within FixedPositionContainer::m_data gets triggered (without calling the constructor). This is
    because the FixedPositionContainer constructor is realized using assignment, mirroring the behavior of vector.
  • Unit tests are added. To test the newly added code under the following scenarios, you can execute it via build/dust/test/dust_moduletests:
    • From an empty container
    • From a single-element container
    • From a multi-element container
    • From a full-capacity container
  • For move cases, the criteria for determining whether FixedPositionContainer::clear() has been executed is based on detecting if the element (ComplexType) has invoked the DTor.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Feel free to add yourself as copyright holder to the files with significant changes :)

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #2069 (5ee7097) into master (443a0d8) will increase coverage by 0.13%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2069      +/-   ##
==========================================
+ Coverage   80.04%   80.18%   +0.13%     
==========================================
  Files         417      418       +1     
  Lines       16055    16113      +58     
  Branches     2255     2265      +10     
==========================================
+ Hits        12852    12920      +68     
+ Misses       2398     2385      -13     
- Partials      805      808       +3     
Flag Coverage Δ
unittests 79.97% <96.55%> (+0.13%) ⬆️
unittests_timing 15.24% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ude/iox/detail/fixed_position_container_helper.hpp 100.00% <100.00%> (ø)
...container/include/iox/fixed_position_container.hpp 100.00% <ø> (ø)
...er/include/iox/detail/fixed_position_container.inl 96.95% <95.23%> (-0.59%) ⬇️

... and 2 files with indirect coverage changes

@Dennis40816 Dennis40816 force-pushed the iox-2052-fixedpositioncontainer-copymove-func-impl branch from 39a9c46 to b0929dd Compare October 31, 2023 12:09
@Dennis40816
Copy link
Contributor Author

I found a weird behavior when using git. I noticed that using git add -p followed by git commit -s doesn't behave as I expect. It changes the content that I've added. For example, I create a file called test.txt.

# Track test.txt
git add test.txt
git commit -s

# Modify separate parts of test.txt, denoted as A and B
git add -p test.txt      # Use y and n (add A and ignore B)
git commit -s

# Both A and B are included in the last commit (though only A was supposed to be included)
git status

So far, I've been using stash to avoid the problem. Am I doing anything wrong?

Dennis40816 added a commit to Dennis40816/iceoryx that referenced this pull request Nov 4, 2023
Signed-off-by: Dennis Liu <[email protected]>

Based on the discussion at
[eclipse-iceoryx#2069 (comment)],
additional tests have been incorporated.

Furthermore, some statistical results have been updated due to
modifications in the `copy_and_move_impl`. For instance, when a
non-empty container is moved to an empty container using the move
assignment, the `MoveCtor` is the function that is invoked, not the
`operator=`. This is because if the slot was previously free, we utilize
placement new to ensure proper functionality. Consequently, it is the
`MoveCtor` that gets called."
@Dennis40816
Copy link
Contributor Author

Dennis40816 commented Nov 4, 2023

The test ran on the Linux platform (Ubuntu 22.04 LTS via WSL) for both C++14 and C++17 with g++11.

@elBoberido
Copy link
Member

@Dennis40816 I don't use git add -p myself so I cannot tell if you did something wrong. Usually I always commit the whole file.

Could you please rebase to master and fix the conflict in the files. I try to start reviewing tomorrow.

@Dennis40816 Dennis40816 force-pushed the iox-2052-fixedpositioncontainer-copymove-func-impl branch from 75a4afa to f825c96 Compare November 6, 2023 01:56
Dennis40816 added a commit to Dennis40816/iceoryx that referenced this pull request Nov 6, 2023
Signed-off-by: Dennis Liu <[email protected]>

Based on the discussion at
[eclipse-iceoryx#2069 (comment)],
additional tests have been incorporated.

Furthermore, some statistical results have been updated due to
modifications in the `copy_and_move_impl`. For instance, when a
non-empty container is moved to an empty container using the move
assignment, the `MoveCtor` is the function that is invoked, not the
`operator=`. This is because if the slot was previously free, we utilize
placement new to ensure proper functionality. Consequently, it is the
`MoveCtor` that gets called."
@elBoberido
Copy link
Member

With the latest merge it seems there is another merge conflict. Please update again. It also seems that the gcc builds are broken. I guess you need to move the specialization to the .inl file

@Dennis40816
Copy link
Contributor Author

Ok, I'll switch to g++ 8.4.0 and take a took what's going on.

Signed-off-by: Dennis Liu <[email protected]>

The `FixedPositionContainer` was enhanced with copy and move
constructors as well as assignment operations. For both copy
and move operations, member variables like `m_size`,
`m_begin_free`, `m_begin_used`, `m_status`, and `m_next` are
handled using copy assignments.

The primary distinction between copy and move is in how
`m_data` is handled.

Specifically, after a move operation:
- The original container's `m_size`, `m_begin_free`, and
  `m_begin_used` are reset.
- However, `m_data`, `m_status`, and `m_next` of the
  original container remain unchanged.

Header declarations have been updated to align with these
modifications.
Signed-off-by: Dennis Liu <[email protected]>

Added unit tests to validate both copy and move semantics
for the `FixedPositionContainer`.

Run `dust/test/dust_moduletests` to view result.
Signed-off-by: Dennis Liu <[email protected]>

Refactor the copy and move constructors of FixedPositionContainer to
leverage the functionality of the respective assignment operators. This
reduces code duplication and ensures consistent behavior across
constructors and assignment operations.

Update tests to reflect the changes in behavior.
Signed-off-by: Dennis Liu <[email protected]>

The changes involved renaming the tests and ensuring the test content
better aligns with the ZOMBIE criteria. The primary object under test is
`SutComplex`. For each constructor or assignment, tests were conducted
on empty containers, single element containers, multiple elements
containers, and full capacity containers. In the case of moves,
additional checks were made to see if the `DTor` of `SutComplex` was
called (from `FixedPositionContainer::clear()`).
Signed-off-by: Dennis Liu <[email protected]>

Add issue 2052 into unreleased document in feature section.
Signed-off-by: Dennis Liu <[email protected]>

Fix nitpick mentioned in PR. Including:

- Replace index value `0U` by `IndexType::First`.
- Change variable to snake_case.
Signed-off-by: Dennis Liu <[email protected]>

Add more strict check(e.g., check full, size, empty) and
rearrange the test code.
Signed-off-by: Dennis Liu <[email protected]>

Fix typo in test case b46d0be7-5977-467e-adc4-2e9adc554fdd.
The result should be true.
Signed-off-by: Dennis Liu <[email protected]>

According to PR discussion, I used the latter method to implement both
ctor and assignment.
Signed-off-by: Dennis Liu <[email protected]>

Add a private function init() to reduce code duplication.
Also, fix the issues mentioned after commit
`b0929dd96287f46d519c634522f2700d0a646221`.
Signed-off-by: Dennis Liu <[email protected]>

We use same test data(e.g., {0U, 1U, 2U, 3U} or `fillSut()` or
`fillSutComplex()` function in two consecutive tests) in
`FixPositionContainer` ctor or assignment related tests.

{56U, 57U, 58U, 59U} replace {0U, 1U, 2U, 3U} in multiple values tests.
Signed-off-by: Dennis Liu <[email protected]>

Rename `init` to `copy_and_move_impl` to clear the function purpose.
Also, move up the declaration of `copy_and_move_impl`.
Signed-off-by: Dennis Liu <[email protected]>

With the previous implementation of `copy_and_move_impl`, non-copyable
classes would encounter compilation failures due to non-compile-time
if-else branching. The compiler would check the deleted copy constructor
and assignment in the `else` branch, even though they would never be
called.

To address this, several helper structs have been implemented to change
runtime if-else branching to compile-time if-else branching for C++14.
If C++17 is used, we can simply employ `if constexpr` to make the
branching compile-time.

The test case `e1cc7c9f-c1b5-4047-811b-004302af5c00`
(UsingMoveCtorAtNonCopyableTypeShouldCompile) verifies that the
implementation works for both C++14 and C++17. The assignment portion
will be added in a subsequent commit.
Signed-off-by: Dennis Liu <[email protected]>

Based on the discussion at
[eclipse-iceoryx#2069 (comment)],
additional tests have been incorporated.

Furthermore, some statistical results have been updated due to
modifications in the `copy_and_move_impl`. For instance, when a
non-empty container is moved to an empty container using the move
assignment, the `MoveCtor` is the function that is invoked, not the
`operator=`. This is because if the slot was previously free, we utilize
placement new to ensure proper functionality. Consequently, it is the
`MoveCtor` that gets called."
Signed-off-by: Dennis Liu <[email protected]>

This commit aims to fix that incorrect member such as `m_begin_free`
and `m_begin_used` might cause unexpected erase error (usually terminate
at line 515).

Hence, the member will be updated after all data are moved or copied
into `this` container, ensuring free list and used list return to
normal status(not broke by copy or move). Then, `erase` can be called
safely.

Finally, the correct information from rhs will be assigned to member at
the bottom of `copy_and_move_impl`.
Signed-off-by: Dennis Liu <[email protected]>

Add a new test case `IteratorsAfterMoveWorkAsExpected`.

Remove previous test cases `ContainerIsNotMovable` and
`ContainerIsNotCopyable`.
@elfenpiff
Copy link
Contributor

@Dennis40816 I reviewed your PR as well, and it looks like a great job!
As soon as the requested additional test cases are in, I would approve it.

@Dennis40816
Copy link
Contributor Author

@elfenpiff Thank you so much! I am happy to contribute to the project.
Btw, occasionally the tests fail (as with the recent builds and tests), and the reasons are still unclear.

@elfenpiff
Copy link
Contributor

@Dennis40816 can you create an issue with the sporadically failing test names. When we have time we can look into it, except when you are eager to dig deeper. 😀

elBoberido
elBoberido previously approved these changes Nov 9, 2023
@elBoberido
Copy link
Member

Btw, occasionally the tests fail (as with the recent builds and tests), and the reasons are still unclear.

I saw only two failures during one CI run. It was the bazel and thread sanitizer and looked like the test file was too large but after triggering the CI again it passed. Did you see more of these? If yes, maybe the file with the tests needs to be split into two.

@elBoberido elBoberido requested a review from elfenpiff November 9, 2023 11:03
@Dennis40816
Copy link
Contributor Author

I saw only two failures during one CI run. It was the bazel and thread sanitizer and looked like the test file was too large but after triggering the CI again it passed.

You're right. But something weird happened.

In build-test-ubuntu-with-thread-sanitizer-clang-latest, all constructors were supposed to be invoked, as the destination container was empty, indicating that constructors should be called initially.

[ RUN      ] FixedPositionContainer_test.UsingCopyCtorFullCapacityContainerPreservesAllElements
/home/runner/work/iceoryx/iceoryx/iceoryx_dust/test/moduletests/test_container_fixed_position_container.cpp:214: Failure
Value of: stats.copyCTor
Expected: is equal to '\n' (10, 0xA)
  Actual: 9 (of type unsigned long)
/home/runner/work/iceoryx/iceoryx/iceoryx_dust/test/moduletests/test_container_fixed_position_container.cpp:216: Failure
Value of: stats.copyAssignment
Expected: is equal to 0
  Actual: 1 (of type unsigned long)
[  FAILED  ] FixedPositionContainer_test.UsingCopyCtorFullCapacityContainerPreservesAllElements (1 ms)

And something similar in build-test-ubuntu-bazel

elBoberido
elBoberido previously approved these changes Nov 9, 2023
@elBoberido elBoberido dismissed their stale review November 9, 2023 15:22

I found the issue with the flaky tests. The problem is that the values of the m_status array are not yet initialized in the copy and move ctors. There are basically two options, either manually initialize them with FREE or copy_and_move_impl needs to be made aware about being used from a ctor.

@elBoberido
Copy link
Member

Since this PR is already quite large and copy_and_move_impl is already a bit complex I think it would be best to add the initialization of m_status in this PR and if you like, you could create a follow up PR to refactor copy_and_move_impl.

I like what you did with the Helper structs and the copy_and_move_impl. Maybe you can come up with a solution which does not need the presence of the assignment operators when the copy/move ctor is used and still share the code.

I haven't fully thought it through but having something like this

enum class SpecialMemberOperation {
    CopyConstructor,
    CopyAssignment,
    MoveConstructor,
    MoveAssignment,
};

Could be used as template parameter for the Helper instead of the boolean and this could then also be used for copy_and_move_impl. If this works out, the Helper structs could be moved to the design module and a small design document could document how they can be used with a copy_and_move_impl.

What do you think?

@elBoberido
Copy link
Member

Oh, and I'm a bit disappointed that the undefined behavior sanitizer did not find the issue with the uninitialized access. I was hoping that it could find issues like this.

@Dennis40816
Copy link
Contributor Author

Dennis40816 commented Nov 9, 2023

Since this PR is already quite large and copy_and_move_impl is already a bit complex I think it would be best to add the initialization of m_status in this PR and if you like, you could create a follow up PR to refactor copy_and_move_impl.

I like what you did with the Helper structs and the copy_and_move_impl. Maybe you can come up with a solution which does not need the presence of the assignment operators when the copy/move ctor is used and still share the code.

I haven't fully thought it through but having something like this

enum class SpecialMemberOperation {
    CopyConstructor,
    CopyAssignment,
    MoveConstructor,
    MoveAssignment,
};

Could be used as template parameter for the Helper instead of the boolean and this could then also be used for copy_and_move_impl. If this works out, the Helper structs could be moved to the design module and a small design document could document how they can be used with a copy_and_move_impl.

What do you think?

Sounds Interesting! I'll try to create a PR(should the issue be created first or not?) about refactoring copy_and_move_impl. We can discuss about the details in that issue or PR.

@Dennis40816
Copy link
Contributor Author

Dennis40816 commented Nov 9, 2023

Do you mean add is_move in if to force copy_and_move_impl using placement new when called by ctor ?

// transfer src data to destination
    for (; rhs_it.to_index() != Index::INVALID; ++i, ++rhs_it)
    {
        //  vvvvvvv
        if (is_move && m_status[i] == SlotStatus::USED)
        {
#if __cplusplus >= 201703L
            if constexpr (is_move)
     
   // ......

   for (; i < CAPACITY; ++i)
   {
        // vvvvvvvv
        if (is_move && m_status[i] == SlotStatus::USED)
        {
            m_data[i].~T();
        }
   }
 // ...

Also, I think m_status will always be updated to correct status after go through these two for loop. So, there's no need to initialize m_status to FREE?

@Dennis40816 Dennis40816 closed this Nov 9, 2023
@elBoberido elBoberido reopened this Nov 9, 2023
@elBoberido
Copy link
Member

Sounds Interesting! I'll try to create a PR(should the issue be created first or not?) about refactoring copy_and_move_impl. We can discuss about the details in that issue or PR.

It's up to you. You can either as additional PR of #2052 and if you are successful create another issue to move the helper classes to the design module and write a small design document. Alternatively, you could create the issue for the helper classes and try to make it work (including a small design document) and afterward adjust the copy_and_move_impl for this class. Whatever fits your workflow best

@elBoberido
Copy link
Member

Do you mean something like below?

// transfer src data to destination
    for (; rhs_it.to_index() != Index::INVALID; ++i, ++rhs_it)
    {
        if (is_move && m_status[i] == SlotStatus::USED)
        // ...

       for (; i < CAPACITY; ++i)
    {
        if (is_move && m_status[i] == SlotStatus::USED)
        {
            m_data[i].~T();
        }

I'm a bit lost 😅
This is code for copy_and_move_impl, right? It won't really work since is_move wil also be true for the move assignment and then the move assignment operator should be used. The issue exists only for the copy and move ctor.

The more I think about it, the more I come to the conclusion that the code of the default ctor needs to be moved to something like init and this should be called at the top of the copy and move ctors. With this change we can merge the PR. Then we have to think if it is possible have some elegant solution for copy_and_move_impl without calling init beforehand

@Dennis40816
Copy link
Contributor Author

Oh my god. You're right. I don't even understand what I was thinking just now. I think I might need to make these modifications tomorrow. I'm sorry for not thinking it through. I think I will follow your suggestion and use an init function (which might do the same thing as the constructor function). Thank you for the reminder.

@elBoberido
Copy link
Member

Oh my god. You're right. I don't even understand what I was thinking just now. I think I might need to make these modifications tomorrow. I'm sorry for not thinking it through. I think I will follow your suggestion and use an init function (which might do the same thing as the constructor function). Thank you for the reminder.

No problem :)
I guess it is already late at your location. Take your time.

Signed-off-by: Dennis Liu <[email protected]>

When we constrcut `FixedPositionContainer`. We need to make
sure that internal status `m_status` be initialized
correctly.

Note that when the constructor (default / copy / move) is
called, the lhs member(`m_status`, `m_next`) are all undefined.
Therefore, `reset_member` should be used in every ctor.
Signed-off-by: Dennis Liu <[email protected]>

It's can be simply replaced by a `for` loop to set m_status
to FREE in copyCtor and assignmentCtor. Therefore, no need
to add a new function.
@elBoberido elBoberido merged commit 7995b3c into eclipse-iceoryx:master Nov 13, 2023
25 checks passed
@elBoberido
Copy link
Member

@Dennis40816 congratulations to your first merged PR in iceoryx. Thanks for the patience with me and I'm looking forward to more contributions from you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants