Exception when the second list element size =1

Hi,

I have the following piece of function. Under the node, there are sul and tdd cells. I try to move them to another node, with two lists, one with sul cells and another with tdd cells.
Now based on some condition is met, each sul is paired with only one tdd cell.

I put the following code, but in the second for loop, the exception is thrown. (The scenario i tested is , 2 sul cells and one tdd cell).

I have shown the line in red. Error is "Cannot decrement begin list iterator"

Code:

---

void UlDlDecoupCellIdentityPlanner::PopulateMultiTechNodePtr2SulTddCellListMap()
{
        ILogicalLayerManager&        rLogLayManager  = DataManager::GetInstance().GetLogicalLayer();
        UserGroupManager&                rUserGrpManager = UserGroupInstance::instance().GetManager();

        ASSERT_ONCE(m_MultiTechNodePtr2SulTddCellListMap.size() == 0);

        // TODO_PRIYA@ SUL to 5g
        const safemap<int, LP5GCELL>& rCellList = m_rUlDlDecoupCellIdPlannerData.GetSulCellList();
        m_pProgressDlg->SetCurrentRangeMax((ULONG)rCellList.size() - 1);

        // Make a list of SUL and TDD cells.

//        for (auto& mapObj : rCellList)
        for(auto& mapObj = rCellList.begin(); mapObj != rCellList.end(); ++mapObj)
        {
                const int iCellKey = mapObj->first;
                const MultiTechCell* pCell = dynamic_cast<const MultiTechCell*>(rLogLayManager.Find(OT_MULTI_TECH_CELL, iCellKey));

                TgNetType eTgNetType = pCell->GetActiveTechnologyMode();
                const TechCellParams* pFiveGTechCellParams = pCell->GetTechnologyParameters(eTgNetType);
                const FiveGCellParams* pFiveGParams = dynamic_cast<const FiveGCellParams*>(pFiveGTechCellParams);        ASSERT_ONCE(pFiveGParams);
                const FiveGCellCarrier* pFiveGCellCarrier = pFiveGParams->GetSingleCarrier();        ASSERT_ONCE(pFiveGCellCarrier);
                const FiveGCarrier* pFiveGCarrier = pFiveGCellCarrier->Get5GCarrier();
                IDataManager* pDataManager = IDataManager::Interface();        ASSERT_ONCE(pDataManager);
                const FiveGFrame* pFiveGFrame = (pFiveGCarrier) ? pFiveGCarrier->Get5gFramePointer(pDataManager) : nullptr;

        //if (!rUserGrpManager.CanModifyObject(*pCell)) continue;

                const auto* pNode = static_cast<const MultiTechNode*>(pCell->GetParent());
                MultiTechNodePtr2SulTddCellListMapIter iterNode = m_MultiTechNodePtr2SulTddCellListMap.find(pNode);

                if (iterNode == m_MultiTechNodePtr2SulTddCellListMap.end())        // If not seen the node before ...
                {
                        SulTddCellListMap rSulTddCellList;
                        MultiTechCellConstPtrList rSulCellList;
                        MultiTechCellConstPtrList rTddCellList;
                        if (pFiveGFrame->GetDuplexMode() == FiveGEnums::FiveGDuplexMode::SUL)
                        {
                                rSulTddCellList.sulCellList.push_back(pCell);
                                m_MultiTechNodePtr2SulTddCellListMap.insert((std::make_pair(pNode, rSulTddCellList)));
                        }
                        else if (pFiveGFrame->GetDuplexMode() == FiveGEnums::FiveGDuplexMode::TDD)
                        {
                                rSulTddCellList.tddCellList.push_back(pCell);
                                m_MultiTechNodePtr2SulTddCellListMap.insert((std::make_pair(pNode, rSulTddCellList)));
                        }
                }
                else
                {
                        SulTddCellListMap& rLsit = iterNode->second;        // .. else update the list of child cells for the node
                        if (pFiveGFrame->GetDuplexMode() == FiveGEnums::FiveGDuplexMode::SUL)
                        {
                                rLsit.sulCellList.push_back(pCell);
                        }
                        else if (pFiveGFrame->GetDuplexMode() == FiveGEnums::FiveGDuplexMode::TDD)
                        {
                                rLsit.tddCellList.push_back(pCell);
                        }
                        else
                        {
                                // NOT HANDLED
                        }
                }

                m_pProgressDlg->IncrementCurrentProgress();
        }

        for (auto& mapObj : m_MultiTechNodePtr2SulTddCellListMap)
        {
                auto nNode = mapObj.first;
                auto nCellList = mapObj.second;
                for (auto& sulCell : nCellList.sulCellList)
                {
                        bool bFirstMatch = false;
                        bool bCanbePaired = false;
//                        for (auto& tddCell : nCellList.tddCellList)
                    for (auto &tddCell = nCellList.tddCellList.begin(); tddCell != nCellList.tddCellList.end(); ++tddCell)
                        {
                                // If can be paired, add the new list and erase TDD cell

        //                        if (CanbePairedBasedOnAzimuth(sulCell,*tddCell))
                                {
                                        auto iterNode = m_MultiTechNodePtr2SulTddGUlDlDecoupCellListMap.find(nNode);

                                        if (iterNode == m_MultiTechNodePtr2SulTddGUlDlDecoupCellListMap.end())
                                        {
                                                SulTddCellList sulTddCellList;

                                                SulTddCellListPairMap pairMap;

                                                pairMap.m_pSulCell = const_cast<MultiTechCell*>(sulCell);
                                                pairMap.m_pTddCell = const_cast<MultiTechCell*>(*tddCell);

                                                sulTddCellList.push_back(pairMap);

                                                m_MultiTechNodePtr2SulTddGUlDlDecoupCellListMap.insert(make_pair(nNode, sulTddCellList));
                                        }
                                        else
                                        {
                                                // else update the list of child cells for the node
                                                SulTddCellList& rLsit = iterNode->second;

                                                SulTddCellListPairMap pairMap;

                                                pairMap.m_pSulCell = const_cast<MultiTechCell*>(sulCell);
                                                pairMap.m_pTddCell = const_cast<MultiTechCell*>(*tddCell);

                                                rLsit.push_back(pairMap);
                                        }

                                        bCanbePaired = true;

                                        // Erase the TDD cell as it cannot be paired again
                                        tddCell = nCellList.tddCellList.erase(tddCell);  //eraseand go to next(erase will return the next iterator)
                                        {
                                                --tddCell;  // as it will be add again in for, so we go back one step
                                        }

                                }
                        }
                        // skip this sul cell.
                        if (bCanbePaired)
                                break;
                }
        }
               
}

---

It will be very helpful, if you can suggest improvements also :)

Thank a lot
pdk

I also tried to write simple program to check the erase and it is working ok. This one i tried on cygwin. (The above program is in visual studio 19).

Basically, when the list has only one element, the erase will return the next element . i.e end() . When it-- is done on end () , what happens ?
In my previous program, i had posted, the erase is returning the begin() . Not sure why ?

Code:

---

int main()
{
        std::list<int> lst(1, 2);

        for(std::list<int>::iterator it = lst.begin(); it != lst.end(); it++)
        {
                std::cout << *it << "\t";

                if (*it == 2)
                {
                        it = lst.erase(it);
                        it--;
                }
        }


        std::cout << "After : ";

        for(std::list<int>::iterator it = lst.begin(); it != lst.end(); it++)
        {
                std::cout << *it << "\t";
        }
}

---

I found a hack:

Instead of :
tddCell--;

I replaced it with the:
if (tddCell == nCellList.tddCellList.begin())
{
break;
}
else
tddCell--;

And it works !!!

The way to erase elements from a container is:

Code:

---

include <list>
#include <iostream>

int main()
{
        std::list<int> lst {3, 2};

        for (auto it = lst.begin(); it != lst.end(); ) {
                std::cout << *it << '\n';

                if (*it == 2)
                        it = lst.erase(it);
                else
                        ++it;
        }

        std::cout << "\nAfter : ";

        for (auto it = lst.begin(); it != lst.end(); ++it) {
                std::cout << *it << '\n';
        }
}

---

Note that the for statement doesn't increment the iterator!

Quote:

---

I found a hack:

---

Yes, because of the wrong way you are doing the erase loop! :thumbd: Your for loop is always incrementing the iterator and you are trying to compensate by decrementing the iterator after the erasure! NO. If you erase you don't alter the iterator after the assignment. If you don't erase then you increment the iterator.

Using very long names within the code makes the code very hard to read and understand! Yes, names should be meaningful but also easy to read. IMO I suggest a limit of about 15.

Using structured bindings can help. Instead of:

Code:

---

for (auto& mapObj : m_MultiTechNodePtr2SulTddCellListMap) {
        auto nNode = mapObj.first;
        auto nCellList = mapObj.second;

---

use:

Code:

---


for (auto& [nNode, nCellList] : m_MultiTechNodePtr2SulTddCellListMap)        {

---

Thanks a lot kaud. :) Very helpful.

Quote:

---

i had posted, the erase is returning the begin()

---

It is returning end(). However:

- for 1 or more elements, end() returns 1 past the last element
- for no elements, end() returns begin()

This is so that the standard iterator for statement:

Code:

---

for (auto iter = cont.begin(); iter != cont.end(); ++iter)

---

works when the container is empty as well as when it has elements.

Quote:

---

Originally Posted by 2kaud

Using very long names within the code makes the code very hard to read and understand! Yes, names should be meaningful but also easy to read. IMO I suggest a limit of about 15.

---

Agreed. I'm a fan of descriptive variables and method names vs. one or two characters, these have gone too far the other way.

Thanks a lot kaud for explaining. I am grateful to get suggestions here, I know I need to work on programming skills. I appreciate your patience and willingness to help . Thanks a lot again.
@Arjay: yes, names are bit far descriptive, as I am just trying to follow template of legacy code.

Thanks a lot again, for inputs, helpful for me to improve.

Note that .insert() within an iterator loop can have a similar - but different- issue and needs to be considered on a case-by-case basis depending upon the container type and location of insertion.

Thanks a lot kaud, for pointing out that.