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

CHANGELOG.md

@@ -56,6 +56,7 @@ All notable changes to this project are documented in this file.
 - Call `positionInterface->setRefSpeeds()` only once when a position reference is set in `YarpRobotControl` (https://github.com/dic-iit/bipedal-locomotion-framework/pull/271)
 - Fix initialization of reference frame for world in `LeggedOdometry` class. (https://github.com/dic-iit/bipedal-locomotion-framework/pull/289)
 - `LeggedOdometry::Impl::updateInternalContactStates()` is now called even if the legged odometry is not initialize. This was required to have a meaningful base estimation the first time `LeggedOdometry::changeFixedFrame()` is called. (https://github.com/dic-iit/bipedal-locomotion-framework/pull/292)
+- Avoid to use the default copy-constructor and copy-assignment operator in `ContactPhaseList` class (https://github.com/dic-iit/bipedal-locomotion-framework/pull/295)
 
 ## [0.1.0] - 2021-02-22
 ### Added

src/Contacts/include/BipedalLocomotion/Contacts/ContactPhaseList.h

@@ -1,7 +1,7 @@
 /**
  * @file ContactPhase.h
- * @authors Stefano Dafarra
- * @copyright 2020 Istituto Italiano di Tecnologia (IIT). This software may be modified and
+ * @authors Stefano Dafarra, Giulio Romualdi
+ * @copyright 2020, 2021 Istituto Italiano di Tecnologia (IIT). This software may be modified and
  * distributed under the terms of the GNU Lesser General Public License v2.1 or any later version.
  */
 
@@ -43,6 +43,59 @@ class ContactPhaseList
     using const_iterator = std::vector<ContactPhase>::const_iterator;
     using const_reverse_iterator = std::vector<ContactPhase>::const_reverse_iterator;
 
+    /**
+     * @brief Constructor.
+     * @note This definition is necessary because the Copy constructor and the Move constructor are
+     * manually defined.
+     */
+    ContactPhaseList() = default;
+
+    /**
+     * @brief Destructor.
+     * @note This definition is necessary to satisfy the rule of five.
+     */
+    ~ContactPhaseList() = default;
+
+    /**
+     * @brief Copy constructor.
+     * @param other another ContactPhaseList.
+     * @note When the copy constructor operator is called  the content of `other.m_contactList` will
+     * be copied in `this->m_contactList`, while `this->m_phases` will be regenerated using the
+     * content of `this->m_contactList`. Notice that Contacts::ContactPhase class stores an
+     * std::unordered_map<std::string, ContactList::const_iterator> called
+     * Contacts::ContactPhase::activeContacts. So copying the content of `other.m_phases` into
+     * `this->m_phases` is wrong since all the iterators stored inside the contact phases should
+     * refer to the lists stored within this class, not the original input lists.
+     */
+    ContactPhaseList(const ContactPhaseList& other);
+
+    /**
+     * @brief Move constructor.
+     * @param other another ContactPhaseList.
+     * @note This definition is necessary to satisfy the rule of five.
+     */
+    ContactPhaseList(ContactPhaseList&& other) = default;
+
+    /**
+     * @brief Copy assignment operator.
+     * @param other another ContactPhaseList.
+     * @note When the copy assignment operator is called  the content of `other.m_contactList` will
+     * be copied in `this->m_contactList`, while `this->m_phases` will be regenerated using the
+     * content of `this->m_contactList`. Notice that Contacts::ContactPhase class stores an
+     * std::unordered_map<std::string, ContactList::const_iterator> called
+     * Contacts::ContactPhase::activeContacts. So copying the content of `other.m_phases` into
+     * `this->m_phases` is wrong since all the iterators stored inside the contact phases should
+     * refer to the lists stored within this class, not the original input lists.
+     */
+    ContactPhaseList& operator=(const ContactPhaseList& other);
+
+    /**
+     * @brief Move assignment operator.
+     * @param other another ContactPhaseList.
+     * @note This definition is necessary to satisfy the rule of five.
+     */
+    ContactPhaseList& operator=(ContactPhaseList&& other) = default;
+
     /**
      * @brief Set the input lists
      * @param contactLists The set of lists to be used for computing the phases.

src/Contacts/src/ContactPhaseList.cpp

@@ -1,31 +1,52 @@
 /**
  * @file ContactPhase.cpp
- * @authors Stefano Dafarra
- * @copyright 2020 Istituto Italiano di Tecnologia (IIT). This software may be modified and
+ * @authors Stefano Dafarra, Giulio Romualdi
+ * @copyright 2020, 2021 Istituto Italiano di Tecnologia (IIT). This software may be modified and
  * distributed under the terms of the GNU Lesser General Public License v2.1 or any later version.
  */
 
 #include <BipedalLocomotion/Contacts/ContactPhaseList.h>
+#include <BipedalLocomotion/TextLogging/Logger.h>
 
-#include <iostream>
-#include <map>
 #include <cassert>
+#include <map>
 
 using namespace BipedalLocomotion::Contacts;
 
-void BipedalLocomotion::Contacts::ContactPhaseList::createPhases()
+ContactPhaseList::ContactPhaseList(const ContactPhaseList& other)
+{
+    this->setLists(other.m_contactLists);
+}
+
+ContactPhaseList& ContactPhaseList::operator=(const ContactPhaseList& other)
+{
+    // if the objects are the same just return this
+    if (&other == this)
+    {
+        return *this;
+    }
+
+    this->setLists(other.m_contactLists);
+    return *this;
+}
+
+void ContactPhaseList::createPhases()
 {
     m_phases.clear();
 
-    std::map<double, std::unordered_map<std::string, ContactList::const_iterator>> activations, deactivations;
+    std::map<double, std::unordered_map<std::string, ContactList::const_iterator>> activations,
+        deactivations;
 
-    for (ContactListMap::iterator list = m_contactLists.begin(); list != m_contactLists.end(); ++list)
+    for (ContactListMap::iterator list = m_contactLists.begin(); list != m_contactLists.end();
+         ++list)
     {
         const std::string& key = list->first;
-        for (ContactList::const_iterator step = list->second.begin(); step != list->second.end(); ++step)
+        for (ContactList::const_iterator step = list->second.begin(); step != list->second.end();
+             ++step)
         {
-            activations[step->activationTime][key] =  step; //By construction, there is only a step given a key and activationTime
-            deactivations[step->deactivationTime][key] =  step;
+            activations[step->activationTime][key] = step; // By construction, there is only a step
+                                                           // given a key and activationTime
+            deactivations[step->deactivationTime][key] = step;
         }
     }
 
@@ -42,63 +63,74 @@ void BipedalLocomotion::Contacts::ContactPhaseList::createPhases()
 
     while ((activations.size() + deactivations.size()) > 1)
     {
-        if ((activations.size() == 0) || (deactivations.begin()->first <= activations.begin()->first))
+        if ((activations.size() == 0)
+            || (deactivations.begin()->first <= activations.begin()->first))
         {
-            //Here I need to remove from the current phase the contacts that are going to end
+            // Here I need to remove from the current phase the contacts that are going to end
             currentPhase.endTime = deactivations.begin()->first;
             m_phases.push_back(currentPhase);
 
             currentPhase.beginTime = deactivations.begin()->first;
 
             for (auto& toBeRemoved : deactivations.begin()->second)
             {
-                currentPhase.activeContacts.erase(toBeRemoved.first); //Erase all the contacts which are deactivativated in this instant
+                currentPhase.activeContacts.erase(toBeRemoved.first); // Erase all the contacts
+                                                                      // which are deactivativated
+                                                                      // in this instant
             }
 
             deactivations.erase(deactivations.begin());
 
             if (activations.size() && (deactivations.begin()->first == activations.begin()->first))
             {
                 currentPhase.activeContacts.insert(activations.begin()->second.begin(),
-                                                   activations.begin()->second.end()); //Add the new contacts to the list.
+                                                   activations.begin()->second.end()); // Add the
+                                                                                       // new
+                                                                                       // contacts
+                                                                                       // to the
+                                                                                       // list.
 
                 activations.erase(activations.begin());
             }
-        }
-        else // (activations.begin()->first < deactivations.begin()->first)
+        } else // (activations.begin()->first < deactivations.begin()->first)
         {
             currentPhase.endTime = activations.begin()->first;
             m_phases.push_back(currentPhase);
 
             currentPhase.beginTime = activations.begin()->first;
             currentPhase.activeContacts.insert(activations.begin()->second.begin(),
-                                               activations.begin()->second.end()); //Add the new contacts to the list.
+                                               activations.begin()->second.end()); // Add the new
+                                                                                   // contacts to
+                                                                                   // the list.
 
             activations.erase(activations.begin());
         }
     }
 
-    assert(deactivations.size() == 1); //Only one element should be left (deactivations and activations were equal in number, but the head of activations was deleted at the beginning).
+    assert(deactivations.size() == 1); // Only one element should be left (deactivations and
+                                       // activations were equal in number, but the head of
+                                       // activations was deleted at the beginning).
     currentPhase.endTime = deactivations.begin()->first;
     m_phases.push_back(currentPhase);
 }
 
-void ContactPhaseList::setLists(const ContactListMap &contactLists)
+void ContactPhaseList::setLists(const ContactListMap& contactLists)
 {
     m_contactLists = contactLists;
     createPhases();
 }
 
-bool ContactPhaseList::setLists(const std::initializer_list<ContactList> &contactLists)
+bool ContactPhaseList::setLists(const std::initializer_list<ContactList>& contactLists)
 {
     m_contactLists.clear();
     for (const ContactList& list : contactLists)
     {
-        std::pair<ContactListMap::iterator, bool> res = m_contactLists.insert(ContactListMap::value_type(list.defaultName(), list));
+        std::pair<ContactListMap::iterator, bool> res
+            = m_contactLists.insert(ContactListMap::value_type(list.defaultName(), list));
 
         if (!res.second)
         {
-            std::cerr << "[ContactPhaseList::setLists] Multiple items have the same defaultName." <<std::endl;
+            log()->error("[ContactPhaseList::setLists] Multiple items have the same defaultName.");
             return false;
         }
     }
@@ -108,7 +140,8 @@ bool ContactPhaseList::setLists(const std::initializer_list<ContactList> &contac
     return true;
 }
 
-const BipedalLocomotion::Contacts::ContactListMap &BipedalLocomotion::Contacts::ContactPhaseList::lists() const
+const BipedalLocomotion::Contacts::ContactListMap&
+BipedalLocomotion::Contacts::ContactPhaseList::lists() const
 {
     return m_contactLists;
 }
@@ -153,7 +186,7 @@ ContactPhaseList::const_reverse_iterator ContactPhaseList::crend() const
     return m_phases.crend();
 }
 
-const ContactPhase &ContactPhaseList::operator[](size_t index) const
+const ContactPhase& ContactPhaseList::operator[](size_t index) const
 {
     return m_phases[index];
 }

src/Contacts/tests/Contacts/ContactPhaseListTest.cpp

@@ -1,7 +1,7 @@
 /**
  * @file ContactPhaseListTest.cpp
- * @authors Stefano Dafarra
- * @copyright 2020 Istituto Italiano di Tecnologia (IIT). This software may be modified and
+ * @authors Stefano Dafarra, Giulio Romualdi
+ * @copyright 2020,2021 Istituto Italiano di Tecnologia (IIT). This software may be modified and
  * distributed under the terms of the GNU Lesser General Public License v2.1 or any later version.
  */
 
@@ -12,6 +12,60 @@
 
 using namespace BipedalLocomotion::Contacts;
 
+TEST_CASE("Rule of five")
+{
+    ContactPhaseList phaseList;
+
+    ContactListMap contactListMap;
+    REQUIRE(contactListMap["left"].addContact(manif::SE3d::Identity(), 0.0, 1.0));
+    REQUIRE(contactListMap["left"].addContact(manif::SE3d::Identity(), 2.0, 5.0));
+    REQUIRE(contactListMap["left"].addContact(manif::SE3d::Identity(), 6.0, 7.0));
+
+    REQUIRE(contactListMap["right"].addContact(manif::SE3d::Identity(), 0.0, 3.0));
+    REQUIRE(contactListMap["right"].addContact(manif::SE3d::Identity(), 4.0, 7.0));
+
+    phaseList.setLists(contactListMap);
+
+    SECTION("Copy constructor")
+    {
+        ContactPhaseList newList(phaseList);
+        phaseList.clear();
+        const ContactListMap& contactListMap = newList.lists();
+        ContactList::const_iterator expectedLeft = contactListMap.at("left").begin();
+        REQUIRE(newList.firstPhase()->activeContacts.at("left")->pose == manif::SE3d::Identity());
+    }
+
+    SECTION("Copy assignment operator")
+    {
+        ContactPhaseList newList;
+        newList = phaseList;
+
+        phaseList.clear();
+        const ContactListMap& contactListMap = newList.lists();
+        ContactList::const_iterator expectedLeft = contactListMap.at("left").begin();
+        REQUIRE(newList.firstPhase()->activeContacts.at("left")->pose == manif::SE3d::Identity());
+    }
+
+    SECTION("Move constructor")
+    {
+        ContactPhaseList newList(std::move(phaseList));
+
+        const ContactListMap& contactListMap = newList.lists();
+        ContactList::const_iterator expectedLeft = contactListMap.at("left").begin();
+        REQUIRE(newList.firstPhase()->activeContacts.at("left")->pose == manif::SE3d::Identity());
+    }
+
+    SECTION("Move assignment operator")
+    {
+        ContactPhaseList newList;
+        newList = std::move(phaseList);
+
+        const ContactListMap& contactListMap = newList.lists();
+        ContactList::const_iterator expectedLeft = contactListMap.at("left").begin();
+        REQUIRE(newList.firstPhase()->activeContacts.at("left")->pose == manif::SE3d::Identity());
+    }
+}
+
 TEST_CASE("ContactPhaseList")
 {
     ContactPhaseList phaseList;
@@ -55,7 +109,6 @@ TEST_CASE("ContactPhaseList")
         ContactList::const_iterator expectedRight = contactListMap.at("right").begin();
         ContactList::const_iterator expectedAdditional = contactListMap.at("additional").begin();
 
-
         ContactPhaseList::const_iterator phase = phaseList.begin();
         REQUIRE(phase->beginTime == 0.0);
         REQUIRE(phase->endTime == 1.0);
@@ -149,5 +202,4 @@ TEST_CASE("ContactPhaseList")
         ok = expectedAdditional == contactListMap.at("additional").end();
         REQUIRE(ok);
     }
-
 }