Insert an object pointer into a map of maps through emplace() does not work

Question

I'm trying to insert a pointer object to a map through emplace() but it does not work.

I've created a simple representation of the problem below. I'm trying to insert to newFooList pointer object type Foo*.

I can't seem to find a way to create a type for FooMap* in std::map<int, FooMap*> m_fooMapList. Should it be done with the new on the second field of the map?

#include <iostream>
#include <utility>
#include <stdint.h>
#include <cstdlib>
#include <map>

class Foo
{
    private:
        int m_foobar;
    public:
        Foo(int value)
        {
            m_foobar = value;
        }
        void setfoobar(int value);
        int getfoobar();
};

class FooMap
{
    private:
        std::map<int, Foo*> m_newFoo;

    public:
        FooMap() = default;
};

class FooMapList
{
    private:
        std::map<int, FooMap*> m_fooMapList;
    public:
        FooMapList() = default;
        void insertFoo(Foo* newFooObj);
};

int Foo::getfoobar(void)
{
    return(m_foobar);
}

void FooMapList::insertFoo(Foo* newFooObj)
{
    if(m_fooMapList.empty())
    {
        std::cout << "m_fooMapList is empty" << std::endl ;
    }

    //m_fooMapList.emplace( newFooObj->getfoobar(), newFooObj  );
    // Need to find a way to insert newFooObj  to m_fooMapList
    m_fooMapList.second = new FooMap;
}

int main() {
    FooMapList newFooList;

    for (auto i=1; i<=5; i++)
    {
        Foo *newFoo = new Foo(i);
        newFoo->getfoobar();
        newFooList.insertFoo(newFoo);
    }

    return 0;
}

On g++ (GCC) 4.8.5 20150623 (Red Hat 4.8.5-28)

$  g++ -std=c++11 -Wall map_of_map.cpp 
map_of_map.cpp: In member function ‘void FooMapList::insertFoo(Foo*)’:
map_of_map.cpp:51:18: error: ‘class std::map<int, FooMap*>’ has no member named ‘second’
     m_fooMapList.second = new FooMap;

Answer 1

m_fooMapList is defined as

    std::map<int, FooMap*> m_fooMapList;

So to insert into it, you need an int and a pointer to FooMap:

    m_fooMapList.emplace(newFooObj->getfoobar(), new FooMap);

---

Having said that, you should use C++ value semantics and rely less on raw pointers:

    std::map<int, FooMap> m_fooMapList; // no pointers

    m_fooMapList.emplace(newFooObj->getfoobar(), {}); // construct objects in-place

That is, instances of FooMap can reside directly in the map itself.

That way you get better performance and avoid memory leaks.

It's also worth looking into smart pointers (e.g. unique_ptr) if you really want to work with pointers.

Answer 2

I am not sure if you need a map structure where the values are pointers to another map. The FooMapList class could be simple

std::map<int, FooMap> m_fooMapList;

On the other hand, the entire play with the row pointer will bring you nothing but a pain on the neck.

In case the use of std::map<int, FooMap*> m_fooMapList; and std::map<int, Foo*> are neccesarry, I would go for smartpointers.

Following is an example code with replacing row pointers with std::unique_ptr and shows how to insert map of Foo s to map in-place. See live here

#include <iostream>
#include <utility>
#include <map>
#include <memory>

class Foo
{
private:
    int m_foobar;
public:
    Foo(int value): m_foobar(value) {}
    void setfoobar(int value) noexcept { m_foobar = value; }
    int getfoobar() const noexcept { return m_foobar; }
};

class FooMap
{
private:
    std::map<int, std::unique_ptr<Foo>> m_newFoo;
    //            ^^^^^^^^^^^^^^^^^^^^
public:
    FooMap() = default;
#if 0 // optional
    // copy disabled
    FooMap(const FooMap&) = delete;
    FooMap& operator=(const FooMap&) = delete;

    // move enabled
    FooMap(FooMap&&) = default;
    FooMap& operator=(FooMap&&) = default;
#endif
    // provide a helper function to insert new Foo to the map of Foo s
    void insertFoo(std::unique_ptr<Foo> newFooObj)
    //             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    {
        std::cout << "inserting to FooMap..." << std::endl;
        m_newFoo.emplace(newFooObj->getfoobar(), std::move(newFooObj)); // construct in place
    }
};

class FooMapList
{
private:
    std::map<int, std::unique_ptr<FooMap>> m_fooMapList;
    //            ^^^^^^^^^^^^^^^^^^^^^^^
public:
    FooMapList() = default;

    void insertFooMap(std::unique_ptr<Foo> newFooObj)
    //               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    {
        if (m_fooMapList.empty())
        {
            std::cout << "m_fooMapList is empty" << std::endl;
        }
        // create FooMap and insert Foo to it.
        FooMap fooMap;
        const auto key = newFooObj->getfoobar();
        fooMap.insertFoo(std::move(newFooObj));

        // finally insert the FooMap to m_fooMapList
        std::cout << "inserting to fooMapList..." << std::endl;
        m_fooMapList.emplace(key, std::make_unique<FooMap>(std::move(fooMap))); // construct in place
    }
};

int main() 
{
    FooMapList newFooList;

    for (auto i = 1; i <= 5; i++)
    {
        auto newFoo = std::make_unique<Foo>(i);
        std::cout << newFoo->getfoobar() << std::endl;
        newFooList.insertFooMap(std::move(newFoo));
    }

    return 0;
}

Output:

1
m_fooMapList is empty
inserting to FooMap...
inserting to fooMapList...
2
inserting to FooMap...
inserting to fooMapList...
3
inserting to FooMap...
inserting to fooMapList...
4
inserting to FooMap...
inserting to fooMapList...
5
inserting to FooMap...
inserting to fooMapList...

Answer 3

You can throw away your do-nothing map classes, stop using pointers, and just

#include <iostream>
#include <utility>
#include <stdint.h>
#include <cstdlib>
#include <map>

class Foo
{
private:
    int m_foobar;
public:
    Foo(int value) : m_foobar(value) { }
    void setfoobar(int value) { m_foobar = value; }
    int getfoobar() const { return m_foobar; }

    // or more simply
    // int foobar;
};

using FooMap = std::map<int, Foo>;

using FooMapMap = std::map<int, FooMap>;

int main() {
    FooMapMap foos;

    for (auto i=1; i<=5; i++)
    {
        foos[i][i] = Foo(i);
    }

    return 0;
}

Do note that the inner map is totally pointless at this stage, as they only ever have one entry

Answer 4

Except if you have a really good reason to do so, avoid to obfuscate things a la Java like this and try to leverage the STL anyway you can. To that end you can use type aliases

using FooMap = std::map<int, Foo*>; // Maybe use a smart pointer instead here?
using FooMapList = std::map<int, FooMap>; // Maybe List is not an appropriate name for a map

Now, you have a Foo element you just created and want to insert it in your map list, to do so you need a way to select in which map in the list you want to insert it. I will assume you'll insert in the first map in the list:

auto FooMap::emplace(int key, Foo* value)
{
    return m_newFoo.emplace(key, value);
}

void FooMapList::insertFoo(Foo* newFooObj)
{
    // If the map for `getfoobar` does not exist yet, operator[] will create it
    auto& mapPtr = m_fooMapList[newFooObj->getfoobar()];
    if (nullptr == mapPtr)
        mapPtr = new FooMap();

    mapPtr->emplace(
        newFooObj->getfoobar(),
        newFooObj
    );
}

Note that I didn't handle memory cleanup. I suggest you try to use smart pointers when applicable (std::unique_ptr and std::shared_ptr)

Answer 5

I have considered valid points from each of the answers to remove pointers and remove useless double level map representations. But the real world abstraction is a much complex problem which involves thousands of objects on the fly which needs to be created and destroyed dynamically. Using pointers seemed a valid approach, but JeJo' approach seemed much better.

I tried to re-use his attempt but with object pointers and the below seems to work. With the below insert functions

In the FooMap class the function would be

void FooMap::insertFoo(Foo* newFooObj)
{
    m_newFoo.emplace(newFooObj->getfoobar(), newFooObj);
}

const std::map<int, Foo*> FooMap::getList()
{
    return m_newFoo;
}

and in FooMapList it would be

void FooMapList::insertFooList(Foo* newFooObj)
{
    std::map <int, FooMap*>::iterator iter;
    FooMap *localFooMap = NULL;
    iter = m_fooMapList.find( newFooObj->getfoobar() );

    if( iter == m_fooMapList.end() )
    {
        localFooMap = new FooMap;
        localFooMap->insertFoo(newFooObj);
        m_fooMapList.emplace(newFooObj->getfoobar(), localFooMap );
    }
    else
    {    
        localFooMap = iter->second;
        localFooMap->insertFoo(newFooObj);
        m_fooMapList.emplace(newFooObj->getfoobar(), localFooMap );
    }
}

const std::map<int, FooMap*> FooMapList::getList()
{
    return m_fooMapList;
}

I would appreciate feedback for this approach too. I will be adding the calls to destructor to clean up the created objects