Implement copy/move constructors and assignment operators for 'FixedPositionContainer' #2052

elBoberido · 2023-10-21T13:02:43Z

Brief feature description

The FixedPositionContainer is neither move nor copy constructible or assignable. Currently it is not required but would be a nice enhancement.

The text was updated successfully, but these errors were encountered:

Dennis40816 · 2023-10-21T16:51:44Z

Hi, @elBoberido I'm interesting in this one. Could I give it a try?

elBoberido · 2023-10-21T17:00:11Z

@Dennis40816 sure. The PR which introduce the class is not merged yet. I would ping you once #2051 is merged. You can of course start with that branch but there might be some force pushes until it is merged.

Dennis40816 · 2023-10-22T04:46:00Z

@elBoberido Sure, I'll be around. Feel free to notify me once it's completed.

elBoberido · 2023-10-25T15:19:28Z

@Dennis40816 it is merged. Looking forward to your PR :)

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.

Dennis40816 · 2023-10-28T07:59:53Z

Hi, elBoberido. As this is my first time making modifications to iceoryx, there might be areas I haven't fully considered, so I'd appreciate your assistance in reviewing. I have initially completed the target implementation and added unit tests.

Dennis40816 · 2023-10-28T08:03:45Z

In the move feature, there's an issue concerning the original container instance after move constructor or move assignment. Currently, I've only modified m_size, m_begin_free, and m_begin_used. However, I realized that not resetting m_next and m_status might lead to errors when the instance is reused. Should we reset the entire container, including m_data, m_next, and m_status, to make it reusable? Or is it better to let the user manually reset it or even prohibit its reuse?

elBoberido · 2023-10-28T11:41:21Z

@Dennis40816 looking at other containers, we also call clear at the end of the move operations. Therefore it would be good to do the same here.

When you think you are done, just open an PR :)

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 `FixPositionContainer::clear()`).

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

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]>

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]> Removed the division symbol '\'. Additionally, a new line has been added between different test sections.