Is it safe to iterate through Collection while remove its member?

[Mark-Joy]

Hello,

Thanks for your great library!! In your example, is it safe to use the below code? Everytime a key is removed, does the under memory layout change, and hence invalid the iterator?

//--- after the update
foreachm(int,key,int,value,m)
{
  if(value%2==0) it.remove();
}

[dingmaotu]

For HashMap, yes, it is safe. The layout does not change until the loop is finished. For other types like LinkedList, Vectorb, etc. no. It is planned to support the latter though.

[Mark-Joy]

Thank you! So it is safe for HashMap but not for Map. Is it safe for HashSet?

[dingmaotu]

It is safe for any Map derived containers (but HashMap is the only one for now) since the remove method is defined on MapIterator. HashSet is a Collection and its Iterator does not have a remove method. But for simple cases like Vector or Vector<SomePointer*>, you can set a special value as a mark for removal, and then use remoeAll method of Collection to do batch removal after the loop. In the future, a similar remove method will be added to the Iterator class.

[Mark-Joy]

Hello again, It seems that HashSet remove() has bug. Sometimes (randomly) remove function did not remove key completely, but instead set key to 0. Update: Currently, to avoid this issue I use Set, not HashSet

Here are some logs from my test:

2018.01.29 23:00:17.161 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273470 on symbol USDJPY
2018.01.29 23:00:17.161 TradeInfo (USDCHF,H1)   Ticket is being checked: 2273435 on symbol USDCHF
2018.01.29 23:00:17.161 TradeInfo (USDCHF,H1)   Ticket is being checked: 2273487 on symbol USDCHF
2018.01.29 23:00:17.161 TradeInfo (USDCHF,H1)   Ticket is being checked: 2273488 on symbol USDCHF
2018.01.29 23:00:17.161 TradeInfo (USDCHF,H1)   Ticket is being checked: 2273437 on symbol USDJPY
2018.01.29 23:00:17.161 TradeInfo (USDCHF,H1)   Ticket is being checked: 2273470 on symbol USDJPY
2018.01.29 23:00:17.201 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273435 on symbol USDCHF
2018.01.29 23:00:17.201 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273487 on symbol USDCHF
2018.01.29 23:00:17.201 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273488 on symbol USDCHF
2018.01.29 23:00:17.201 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273437 on symbol USDJPY
2018.01.29 23:00:17.201 TradeInfo (USDJPY,H1)   Ticket is being checked: 2273470 on symbol USDJPY
2018.01.29 23:00:17.417 TradeInfo (USDCHF,H1)   Ticket is removed: 2273435
2018.01.29 23:00:17.417 TradeInfo (USDCHF,H1)   Ticket is being checked: 0 on symbol USDCHF
2018.01.29 23:00:17.417 TradeInfo (USDCHF,H1)   invalid pointer access in 'TradeInfo.mq5' (546,13)
2018.01.29 23:00:17.417 TradeInfo (USDJPY,H1)   Ticket is removed: 2273435
2018.01.29 23:00:17.417 TradeInfo (USDJPY,H1)   Ticket is being checked: 0 on symbol USDCHF

My code look like this:

Tickets is HashSet<ulong>

foreachm(ulong, ticket, MyPositionInfo*, posInfo, ticketPosInfo)
    {
      // If Ticket is not available, it means that Position has been closed.
      // Then remove the PosInfo in TicketPosInfo and delete the posInfo.
      // Remove Ticket in symbolPosInfo[sym].
      // If there is no ticket left, then delete and remove symbolPosInfo[sym]
      if (!PositionSelectByTicket(ticket))
      {
        string sym = posInfo.iSymbol;
        //////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////
        // TICKET IS REMOVED FROM symbolPosInfo[sym].Tickets
        bool result = symbolPosInfo[sym].Tickets.remove(ticket);
        //////////////////////////////////////////////////////////////////
        //////////////////////////////////////////////////////////////////

        RemoveSymbolPositionsIfEmpty(sym);

        MyPositionInfo::RemovePositionByTicket(ticket);

        PrintFormat("Ticket is removed: %d  %d", ticket, result);
      }
    }

    //Update Calculating Profit, Volume, Position count for SymbolPositions
    foreachm(string, sym, SymbolPositionsInfo*, symPosInfo, symbolPosInfo)
    {
      //////////////////////////////////////////////////////////////
      // CHECK TICKET FROM symbolPosInfo[sym].Tickets
      foreachv(ulong, ticket, symPosInfo.Tickets)
      {
        PrintFormat("Ticket is being checked: %d on symbol %s", ticket, sym);
        MyPositionInfo * posInfo = ticketPosInfo[ticket];
......

I compare HashSet and HashMap, this is just my guess Shall the below code from HashSet

 if(m_owned)
     {
      // delete possble pointers
      SafeDelete(m_entries.getKey(ix));
     }

be:

SafeDelete(m_entries.getKey(ix));
 if(m_owned)
     {
      // delete possble pointers
      //SafeDelete(m_entries.getKey(ix));
     }

[dingmaotu]

Thanks for reporting this. The m_owned and SafeDelete are not related to this issue since HashSet elements are not pointers. I suppose it is a bug of HashSetIterator. I submitted a commit just now and could you please pull the latest code and test to see if the problem gets fixed?

[Mark-Joy]

Thank you!! It is working great now! No error so far