map of pointers inside another map raise exception

Hi, All

I have created my own std::map wrapper because it makes some of my tasks simpler this way.
The code for the wrapper:

//---------------------------------------------------------------------------
#ifndef MapListH
#define MapListH
//---------------------------------------------------------------------------
#include <System.Classes.hpp>
#include <System.SysUtils.hpp>
#include "ClangCpp.h"
#include <string.h>
#include <map>
//---------------------------------------------------------------------------
//New Events
//---------------------------------------------------------------------------
template <class Key, class Value>
class TMapList
{
protected:
typedef std::pair<Key,Value> PairType;
typedef void __fastcall (__closure *TForEachItem)(Key Index, Value Item);
typedef void __fastcall (__closure *TItemClear)(Key Index, Value Item);
typedef typename std::map<Key,Value>::iterator TMapListIterator;

private:
//Events
TItemClear FOnItemClear;

//---------------------------------------------------------------------------
protected:
//Setters/Getters
virtual int __fastcall GetCount() const;
virtual void __fastcall SetElement(Key Index, const Value& Item);
virtual Value __fastcall GetElement(Key Index) const;
virtual void __fastcall SetElementByIndex(int Index, const Value& Item);
virtual Value __fastcall GetElementByIndex(int Index) const;
virtual Key __fastcall GetKeyByIndex(int Index) const;
virtual Key __fastcall GetKeyByValue(Value Item) const;

//Do Events Methods
virtual void __fastcall DoItemClear(Key Index, Value Item);

//---------------------------------------------------------------------------
public:
//Fields/Properties
mutable std::map< Key, Value> Map;

mutable typename std::map<Key,Value>::iterator Iter;
__property int Count = {read = GetCount};
__property Value Item[Key Index] = {read = GetElement, write = SetElement};
__property Value ItemByIndex[int Index] = {read = GetElementByIndex, write = SetElementByIndex};
__property Key KeysByIndex[int Index] = {read = GetKeyByIndex};
__property Key KeyByValue[Value Item] = {read = GetKeyByValue};

//Constructor / Destructor.
__fastcall TMapList() {FOnItemClear = nullptr;};
virtual __fastcall ~TMapList(void);

//Methods
virtual void __fastcall First() const;
virtual void __fastcall Last() const;
virtual void __fastcall IncIter();
virtual void __fastcall Add(Key Index, const Value& Item);
virtual bool __fastcall Exists(Key Index) const;
virtual typename std::map<Key,Value>::iterator __fastcall FindIter(Key Index) const;
virtual bool __fastcall Find(Key Index, Value& Item) const;
virtual bool __fastcall Find(Key Index) const;
virtual bool __fastcall TillEnd() const;
virtual bool __fastcall ValueExists(Value Item) const;
virtual Value __fastcall Delete(Key Index);
virtual bool __fastcall Empty() const;
//Methods
virtual void __fastcall ForEachItem(TForEachItem ForEachItemEvent);

virtual void __fastcall Clear();

inline bool operator == (const TMapList& MapListValue) const
{
return (Map == MapListValue.Map);
}
//--------------------------------

//Events
__property TItemClear OnItemClear= {read = FOnItemClear, write = FOnItemClear};
};
//---------------------------------------------------------------------------
typedef std::map<String,String> TStrsMap;

typedef std::pair<int,int> TIntToIntPair;
typedef std::pair<int,String> TIntToStrPair;
typedef std::pair<String,String> TStrToStrPair;
typedef std::pair<String,int> TStrToIntPair;

typedef TMapList<int,int> TIntToIntMap;
typedef TMapList<int,String> TIntToStrMap;
typedef TMapList<String,String> TStrToStrMap;
typedef TMapList<String,int> TStrToIntMap;
//---------------------------------------------------------------------------
template <class Key, class Value>
int __fastcall TMapList<Key,Value>::GetCount() const
{
return Map.size();
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::SetElement(Key Index, const Value& Item)
{
Map[Index] = Item;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
Value __fastcall TMapList<Key,Value>::GetElement(Key Index) const
{
return Map[Index];
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::SetElementByIndex(int Index, const Value& Item)
{
Iter = Map.begin();
std::advance(Iter, Index);
Iter->second = Item;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
Value __fastcall TMapList<Key,Value>::GetElementByIndex(int Index) const
{
Iter = Map.begin();
std::advance(Iter, Index);
return Iter->second;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
Key __fastcall TMapList<Key,Value>::GetKeyByIndex(int Index) const
{
Iter = Map.begin();
std::advance(Iter, Index);
return Iter->first;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
Key __fastcall TMapList<Key,Value>::GetKeyByValue(Value Item) const
{
Key res;

for (Iter = Map.begin(); Iter != Map.end(); ++Iter )
   {
   if (Iter->second == Item)
      {
      res = Iter->first;
      break;
      }
   }
return res;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::DoItemClear(Key Index, Value Item)
{
if (FOnItemClear)
   FOnItemClear(Index, Item);
}
//---------------------------------------------------------------------------
template <class Key, class Value>
__fastcall TMapList<Key,Value>::~TMapList(void)
{
Clear();
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::First() const
{
this->Iter = this->Map.begin();
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::Last() const
{
this->Iter = this->Map.end();
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::IncIter()
{
++Iter;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::TillEnd() const
{
return (this->Iter != this->Map.end());
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::Add(Key Index, const Value& Item)
{
Map.insert(std::make_pair( Index, Item));
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::Exists(Key Index) const
{
return Map.count(Index);
}
//---------------------------------------------------------------------------
template <class Key, class Value>
typename std::map<Key,Value>::iterator __fastcall TMapList<Key,Value>::FindIter(Key Index) const
{
return Map.find(Index);
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::Find(Key Index, Value& Item) const
{
Iter = Map.find(Index);

if (Iter != Map.end())
   {
   Item = Iter->second;
   return true;
   }
else
   return false;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::Find(Key Index) const
{
Iter = Map.find(Index);
return (Iter != Map.end());
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::ValueExists(Value Item) const
{
for (Iter = Map.begin(); Iter != Map.end(); ++Iter )
   {
   if (Iter->second == Item)
      return true;
   }
return false;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
Value __fastcall TMapList<Key,Value>::Delete(Key Index)
{
Value temp = Item[Index];
Map.erase(Index);
return temp;
}
//---------------------------------------------------------------------------
template <class Key, class Value>
bool __fastcall TMapList<Key,Value>::Empty() const
{
return (GetCount() == 0);
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::ForEachItem(TForEachItem ForEachItemEvent)
{
for (Iter=Map.begin(); Iter !=Map.end(); ++Iter)
   {
   ForEachItemEvent(Iter->first, Iter->second);
   }
}
//---------------------------------------------------------------------------
template <class Key, class Value>
void __fastcall TMapList<Key,Value>::Clear()
{
if (Count > 0)
   {
   if (FOnItemClear)
      {
      for (Iter=Map.begin(); Iter !=Map.end(); ++Iter)
         {
         DoItemClear(Iter->first, Iter->second);
         }
      }

   Map.clear();
   }
}
//---------------------------------------------------------------------------
//---------------------------------------------------------------------------
template <class Key, class Value>
class TPtrMapList : public TMapList<Key, Value>
{
public:
virtual __fastcall ~TPtrMapList(void)
{
this->Clear();
}

virtual void __fastcall Clear()
{
if (!this->Map.empty())
   {
   this->Iter = this->Map.begin();

   for (; this->Iter != this->Map.end(); ++this->Iter)
      {
      if (this->Iter->second != nullptr)
         delete this->Iter->second;
      }
   }

this->Map.clear();
}
};
//---------------------------------------------------------------------------
#endif

As you can see I have 2 TMapList classes one for normal types and another for pointers.
Then I have this code which raises an error when I user a TPtrMapList inside another TMapList like below:

//Header
struct TTestObject
{
int ID;
String Name;
TStrToStrMap ActionsConditions;
};
typedef TMapList<String,TTestObject*> TTestObjectList;
typedef TMapList<String,TTestObjectList> TResourcesMapList;

class TForm1 : public TForm
{
__published:
...
public:
TResourcesMapList ResourcesMapList;

void __fastcall TForm1::Btn1Click(TObject *Sender)
{
TTestObjectList ObjectList;
TTestObject* Obj = new TTestObject;

Obj->ID = 1;
Obj->Name = "AAA";

Obj->ActionsConditions.Add("CCC","");

ObjectList.Add("DDD",Obj);

ResourcesMapList.Add("Test", ObjectList); // Error raised here I think the destructor is called but why

I tried using references instead of pointers before but because I am filling the "TTestObjectList" with derived classes from "TTestObject" it was giving me errors as well during casting from base object inside the map to derived class, I tried dynamic_cast, static_cast, reinterpret_cast but nothing works always raises an exception when I use ay new data member from the derived classes.

Now this works fine with shared_ptr instead of using normal pointers but I want to know what am I doing wrong?

I am passing ObjectList as "const reference" shouldn't this pass the same object not make a copy, then why the destructor is called for the TPtrMapList at this point when I add it into another map?

Thanks

Parents
No Data
Reply
  • As you can see I have 2 TMapList classes one for normal types and another for pointers.
    Then I have this code which raises an error when I user a TPtrMapList inside another TMapList

    This is caused by a classic Rule of 3/5/0 violation, so it makes sense why storing raw pointers is cause problems, and why storing std::shared_ptr objects instead solves it.

    Specifically, your TPtrMapList class lacks proper copy/move constructors and copy/move assignment operators to make DEEP COPIES of its std::map data, so instead it makes SHALLOW COPIES, which is bad news when that data is raw pointers to allocated objects.  It means you can end up with multiple TPtrMapList instances holding pointers to the same objects, so when all of those TPtrMapList instances are destroyed or cleared or what-not, they try to delete the same objects in memory multiple times, causing crashes.

    When you call ObjectList.Add("DDD",Obj), that Add() method makes a shallow copy of the Obj variable when inserting it into ObjectList's std::map.  That copied TPtrMapList holds the same raw pointers as the Obj variable does.  And then, when you call ResourcesMapList.Add("Test", ObjectList), that Add() method makes a shallow copy of the ObjectList variable when inserting it into ResourcesMapList's std::map.  So you end up with even more copies of the raw pointers.  Eventually, the Obj and ObjectList variables will go out of scope, destroying the pointed-to TTestObject objects, which the std::maps are still pointing at.

    If you don't want to store std::shared_ptr (or std::unique_ptr) objects (which you should in this case, DON'T use raw pointers in modern C++ coding), then you need to make TPtrMapList 'delete' its default copy constructor and copy assignment operator to avoid those shallow copies of raw pointers altogether, and to explicitly implement a move constructor and move assignment operator to move the inner maps around without making copies of them, eg:

    template <class Key, class Value>
    class TPtrMapList : public TMapList<Key, Value>
    {
    public:
        TPtrMapList() = default;
    
        TPtrMapList(const TPtrMapList&) = delete;
        TPtrMapList& operator=(const TPtrMapList&) = delete;
        
        TPtrMapList(TPtrMapList &&src)
        {
            >Map = std::move(src.Map);
        }
        
        TPtrMapList& operator=(TPtrMapList &&rhs)
        {
            Map = std::move(rhs.Map);
            return *this;
        }
    
        virtual __fastcall ~TPtrMapList(void)
        {
            Clear();
        }
    
        virtual void __fastcall Clear()
        {
            for (auto &value : Map)
            {
                delete value;
            }
    
            Map.clear();
        }
    };

    You will also have to update TMapList to use move semantics when inserting new elements into its std::map, eg:

    template <class Key, class Value>
    void __fastcall TMapList<Key,Value>::SetElement(Key Index, Value &&Item)
    {
    	Map[Index] = std::move(Item);
    }
    
    template <class Key, class Value>
    void __fastcall TMapList<Key,Value>::SetElementByIndex(int Index, Value &&Item)
    {
    	std::next(Map.begin(), Index)->second = std::move(Item);
    }
    
    template <class Key, class Value>
    void __fastcall TMapList<Key,Value>::Add(Key Index, Value &&Item)
    {
    	Map.insert(std::make_pair(Index, std::move(Item)));
    }
    

    But then you will also run into similar problems with methods like TMapList::GetElement() and TMapList::GetElementByIndex(), which return std::map values *by value* instead of *by reference*, thus making copies.  So you will have to address those issues, too.

    The better option is to simply get rid of TPtrMapList altogether and use smart pointers with TMapList.  This is exactly the kind of situation that smart pointers were invented for.

Children