Document proper usage of BipedalLocomotion::Contacts::ContactList::editContact()

[LoreMoretti]

The function BipedalLocomotion::Contacts::ContactList::editContact might cause segmentation fault if used within loops, such as for loops.

For example the following pseudo-code:

       newContactList =  myContactList;

        for (auto it = myContactList.begin(); it != myContactList.end(); it++)
        {
            BipedalLocomotion::Contacts::PlannedContact newContact{};

            // ===== do something meaningfull with newContact      

            newContactList.editContact(it, newContact);
        }

@S-Dafarra pointed out the it could be due to these lines.

It should be documented that this function should not be used within loops.

[S-Dafarra]

The problem is that https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/src/Contacts/src/ContactList.cpp#L208-L209 might invalidate existing iterators.

It should be at least documented. It would be even better if we avoid to have this issue at all

[LoreMoretti]

What about something as follows:

1. Change the editContact signature, from

https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/src/Contacts/include/BipedalLocomotion/Contacts/ContactList.h#L207-L208

to bool editContact(const_iterator& element, const BipedalLocomotion::Contacts::PlannedContact& newContact);

2. Changing the erase and insert lines from: https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/src/Contacts/src/ContactList.cpp#L208-L209

to

    element = m_contacts.erase(element);
    element = m_contacts.insert(element, newContact);

This should have the effect of changing the const_iterator element (which is invalidated by the erase function) to a new valid one, which points at the inserted element.

Could it work?

If it works, then we should change also the way editContact is called within forceSampleTime:

https://github.com/ami-iit/bipedal-locomotion-framework/blob/e341b70f188562b5efdc717a2bdcfde8eb92aa77/src/Contacts/src/ContactList.cpp#L336

[S-Dafarra]

This should have the effect of changing the const_iterator element (which is invalidated by the erase function) to a new valid one, which points at the inserted element.

Could it work?

Unfortunately, I don't think so. The erase can invalidate all existing iterators, not just the one we are passing.

[LoreMoretti]

Unfortunately, I don't think so. The erase can invalidate all existing iterators, not just the one we are passing.

I see your point, which in fact is also reported in the c++ documentation%20and%20references%20to%20the%20elements%20at%20or%20after%20the%20point%20of%20the%20erase%20are%20invalidated.).

Though, in the erase documentation, I see the following example:

std::vector<int> c{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};

// Erase all even numbers
    for (std::vector<int>::iterator it = c.begin(); it != c.end();)
    {
        if (*it % 2 == 0)
            it = c.erase(it);
        else
            ++it;
    }

where iterators get erased within a for loop, which cycles on the iterators themselves.

[S-Dafarra]

I see. In any case, this would fix the issue with the iterator to the item that has been edited. The problem is if you have other iterators around. Those could also become invalid.