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 Refactor copy_and_move_impl #2105

Conversation

Dennis40816
Copy link
Contributor

@Dennis40816 Dennis40816 commented Nov 20, 2023

Refactor FixedPositionContainer::copy_and_move_impl by new struct SpecialCreationHelper and enum class SpecialCreationOperations with C++17 features.

Pre-Review Checklist for the PR Author

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

Notes for Reviewer

I implement a prototype named SpecialCreationHelper according to previous discussion.
If it looks good to you, I'll create another issue to move the helper class to design module.

Btw, I have no idea if move_or_copy and move_or_copy_it are needed or not. These functions currently cause twice time moveCtor calls problem (See example). Currently, I directly use if constexpr to pass correct value. If you think they are unnecessary, I will delete related comment and code.

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

Refactor copy_and_move_impl by new struct `SpecialCreationHelper` and
enum class `SpecialCreationOperations` with C++17 features.

Deleter __cplusplus branchs.

Signed-off-by: Dennis Liu <[email protected]>
@Dennis40816 Dennis40816 marked this pull request as draft November 20, 2023 06:01
@Dennis40816 Dennis40816 marked this pull request as ready for review November 20, 2023 06:15
@Dennis40816 Dennis40816 changed the title [WIP] iox-#2052 Refactor copy_and_move_impl iox-#2052 Refactor copy_and_move_impl Nov 21, 2023
When we commit, Clang Format should have already been applied once.
However, the CI process failed. I suspect it's due to an issue with
inappropriate new lines.

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

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2105 (7bd507e) into master (51a018a) will increase coverage by 0.02%.
Report is 27 commits behind head on master.
The diff coverage is 95.45%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2105      +/-   ##
==========================================
+ Coverage   80.28%   80.30%   +0.02%     
==========================================
  Files         417      418       +1     
  Lines       16121    16143      +22     
  Branches     2250     2252       +2     
==========================================
+ Hits        12942    12963      +21     
- Misses       2384     2385       +1     
  Partials      795      795              
Flag Coverage Δ
unittests 80.09% <95.45%> (+0.02%) ⬆️
unittests_timing 15.28% <0.00%> (-0.03%) ⬇️

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

Files Coverage Δ
...er/include/iox/detail/fixed_position_container.inl 96.96% <100.00%> (-0.02%) ⬇️
...container/include/iox/fixed_position_container.hpp 100.00% <ø> (ø)
...ude/iox/detail/fixed_position_container_helper.hpp 91.66% <88.88%> (ø)

... and 8 files with indirect coverage changes

@Dennis40816
Copy link
Contributor Author

@elBoberido Please review this when you have a moment. Thank you :)

@elBoberido
Copy link
Member

@Dennis40816 it's on my todo list for today. I hope I find some time :)

@elBoberido elBoberido self-requested a review November 21, 2023 17:57
@elBoberido elBoberido added the refactoring Refactor code without adding features label Nov 21, 2023
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.

Nice. Looks good overall. I think this could be moved to iceoryx_hoofs/design in a follow up PR. Then the discussion about names might get hot :)

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.

Great job. Like already mentioned, the helper struct could be reused by other classes and it would be great if you had some time to move it to iceoryx_hoofs/design and write a small design document in doc/design. Just copy _template.md. If a sections does not apply just delete it.

I think it could be done as part of #1686. If you want to test the abstraction on another class then #1517 would be ideal :)

@elBoberido elBoberido merged commit 5872051 into eclipse-iceoryx:master Nov 22, 2023
28 checks passed
@Dennis40816
Copy link
Contributor Author

No problem. I'll create an new issue about moving it to iceoryx_hoofs/design this weekend!

@Dennis40816 Dennis40816 deleted the iox-2052-refactor-copy_and_move_impl branch November 22, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactor code without adding features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement copy/move constructors and assignment operators for 'FixedPositionContainer'
2 participants