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

Fix copy-constructor and copy-assignment operator in ContactPhaseList class #295

Merged
merged 5 commits into from
May 5, 2021

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented May 5, 2021

This Implements the copy-constructor and copy-assignment operator for the ContactPhaseList class.

ContactPhaseList has an attribute called m_phases

https://github.com/dic-iit/bipedal-locomotion-framework/blob/d916b56f71eca03a7d1e519e4f13d785665b79ed/src/Contacts/include/BipedalLocomotion/Contacts/ContactPhaseList.h#L37

where ContactPhase has a std::unordered_map of active contacts during a given phase.

https://github.com/dic-iit/bipedal-locomotion-framework/blob/d916b56f71eca03a7d1e519e4f13d785665b79ed/src/Contacts/include/BipedalLocomotion/Contacts/ContactPhase.h#L37-L40

Accordingly to the ContactPhaseList documentation

https://github.com/dic-iit/bipedal-locomotion-framework/blob/d916b56f71eca03a7d1e519e4f13d785665b79ed/src/Contacts/include/BipedalLocomotion/Contacts/ContactPhaseList.h#L28-L31

all the iterators stored in this->m_phases should refer to this->m_contactList.
However when the default copy-constructor/copy-assignment operator are colled the content of other.m_phases is copied into this->m_phases and consequentially the iterators stored in this->m_phases will refer to other.m_contactLists.

I wrote an ad-hoc test to spot the bug. The test fails in Ubuntu 20.04 if Valgrind is used. Please check here.


In order to satisfy the rule of five the move-constructor move-assignment operator and destructor member functions must be defined.


I also introduced TextLogging in ContactPhaseList

Thank you @prashanthr05 for helping me in debugging 😃

@GiulioRomualdi GiulioRomualdi added the bugfix Fix a critical bug in the existing code label May 5, 2021
@GiulioRomualdi GiulioRomualdi self-assigned this May 5, 2021
Copy link
Collaborator

@prashanthr05 prashanthr05 left a comment

Choose a reason for hiding this comment

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

Minor comments.

@GiulioRomualdi GiulioRomualdi force-pushed the fix/CopyCtr_CopyAssign_ContactPhaseList branch from 5ec37f1 to dcfe79c Compare May 5, 2021 09:27
@GiulioRomualdi
Copy link
Member Author

@prashanthr05 I fixed the typos and I rebased

@prashanthr05
Copy link
Collaborator

Green light from me for the merge.

Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

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

Thanks!

@GiulioRomualdi GiulioRomualdi merged commit c6f901f into master May 5, 2021
@GiulioRomualdi GiulioRomualdi deleted the fix/CopyCtr_CopyAssign_ContactPhaseList branch May 5, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a critical bug in the existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants