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

Jump to bottom

iox-#2414 Reset index after move construction #2413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

iox-#2414 Reset index after move construction #2413

wants to merge 1 commit into from

Conversation

yuanxingyang
Copy link

@yuanxingyang yuanxingyang commented Jan 16, 2025 •
edited
Loading

Notes for Reviewer

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. Assign PR to reviewer

Checklist for the PR Reviewer

  • Consider a second reviewer for complex new features or larger refactorings
  • 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

  • Closes TBD

@elfenpiff
Copy link
Contributor

@yuanxingyang Thank you for your contribution!

Please follow the contribution rules: https://github.com/eclipse-iceoryx/iceoryx/blob/main/CONTRIBUTING.md

  1. Sign the eclipse contributor agreement with the same account you committed
  2. Create an issue first
  3. Adjust every commit message so that it has the prefix iox-#??? where ??? is the issue number

When this is done I am happy to merge your contribution

@elBoberido
Copy link
Member

@yuanxingyang the process is a bit cumbersome for an initial contributor. The reason for all the 'paperwork' is because we are an Eclipse project (therefore the ECA) and we aim for safety certification (therefore the issue for traceability).

To ease your first contribution, I created #2414 for you. Please use this issue number for your commit. You can change the commit message with git commit --amend. For more regular contributions, the git hooks can be used:
https://github.com/eclipse-iceoryx/iceoryx/blob/main/tools/git-hooks/Readme.md

If possible, it would be great to create a tests for the bugfix.

@elBoberido elBoberido added the bugfix Solves a bug label Jan 17, 2025
elBoberido
elBoberido reviewed Jan 17, 2025
View reviewed changes
iceoryx_hoofs/vocabulary/include/iox/detail/variant.inl
@@ -97,6 +97,7 @@ inline constexpr variant<Types...>::variant(variant&& rhs) noexcept
if (m_type_index != INVALID_VARIANT_INDEX)
{
internal::call_at_index<0, Types...>::moveConstructor(m_type_index, &rhs.m_storage, &m_storage);
rhs.m_type_index = INVALID_VARIANT_INDEX;
Copy link
Member

Choose a reason for hiding this comment

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

Can you check if this also required for the move assignment operator?

Copy link
Author

Choose a reason for hiding this comment

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

Hello @elBoberido
I have submitted an additional patch. Please review it

Copy link
Member

Choose a reason for hiding this comment

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

@yuanxingyang looks good. Let's see what the CI tells us. Maybe a suppression for a warning accessing a moved object needs to be added.

Did you already signed the ECA? It can take some time until everything is synced.

Can you also please add the issue number to the first commit?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it seems you need to run clang-format on the tests

Copy link
Member

Choose a reason for hiding this comment

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

... and forget my question regarding the ECA 😅

Copy link
Author

Choose a reason for hiding this comment

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

Oh, it seems you need to run clang-format on the tests

I have formatted the latest commit using it

Copy link
Author

Choose a reason for hiding this comment

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

@yuanxingyang looks good. Let's see what the CI tells us. Maybe a suppression for a warning accessing a moved object needs to be added.

Did you already signed the ECA? It can take some time until everything is synced.

Can you also please add the issue number to the first commit?

I have forcefully combined two commits into one and added the commit message according to the guidelines.

@yuanxingyang yuanxingyang changed the title Reset index after move construction iox-#2414 Reset index after move construction Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elBoberido elBoberido elBoberido left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bugfix Solves a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuanxingyang @elfenpiff @elBoberido