Shallow/deep copy of std::map

Question

How would I best implement these? I thought of something like this:

    using namespace std;

    shape_container
    shape_container::clone_deep () const
    {
        shape_container* ptr = new shape_container();
        copy( data.begin(), data.end(), (*ptr).begin() );
        return *ptr;
    }

    shape_container
    shape_container::clone_shallow () const
    {
        return *( new shape_container(*this) );
    }

The member data is defined as follows:

    std::map<std::string, shape*> data;

This doesn't work, unfortunately. Here's the compiler errors, I don't really understand them:

    g++ -Wall -O2 -pedantic -I../../UnitTest++/src/ -I./libfglwin/include/ -I. -c shape_container.cpp -o shape_container.o
    /usr/include/c++/4.2.1/bits/stl_pair.h: In member function ‘std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>& std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>::operator=(const std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>&)’:
    /usr/include/c++/4.2.1/bits/stl_pair.h:69:   instantiated from ‘static _OI std::__copy<<anonymous>, <template-parameter-1-2> >::copy(_II, _II, _OI) [with _II = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, _OI = std::_Rb_tree_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, bool <anonymous> = false, <template-parameter-1-2> = std::bidirectional_iterator_tag]’
    /usr/include/c++/4.2.1/bits/stl_algobase.h:315:   instantiated from ‘_OI std::__copy_aux(_II, _II, _OI) [with _II = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, _OI = std::_Rb_tree_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >]’
    /usr/include/c++/4.2.1/bits/stl_algobase.h:340:   instantiated from ‘static _OI std::__copy_normal<<anonymous>, <anonymous> >::__copy_n(_II, _II, _OI) [with _II = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, _OI = std::_Rb_tree_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, bool <anonymous> = false, bool <anonymous> = false]’
    /usr/include/c++/4.2.1/bits/stl_algobase.h:401:   instantiated from ‘_OutputIterator std::copy(_InputIterator, _InputIterator, _OutputIterator) [with _InputIterator = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, _OutputIterator = std::_Rb_tree_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >]’
    shape_container.cpp:70:   instantiated from here
    /usr/include/c++/4.2.1/bits/stl_pair.h:69: error: non-static const member ‘const std::basic_string<char, std::char_traits<char>, std::allocator<char> > std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>::first’, can't use default assignment operator
    /usr/include/c++/4.2.1/bits/stl_algobase.h: In static member function ‘static _OI std::__copy<<anonymous>, <template-parameter-1-2> >::copy(_II, _II, _OI) [with _II = std::_Rb_tree_const_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, _OI = std::_Rb_tree_iterator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*> >, bool <anonymous> = false, <template-parameter-1-2> = std::bidirectional_iterator_tag]’:
    /usr/include/c++/4.2.1/bits/stl_algobase.h:268: note: synthesized method ‘std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>& std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>::operator=(const std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, shape*>&)’ first required here 

Somehow this looks unnecessarily complicated to me. Is that true and can I make it better?

BTW, I have clone() methods in the classes I derived from shape. Perhaps I can use them for the clone_deep method? Are they ok? They look something like this:

    class shape
    {
        public:
            /* Many methods. */
            virtual shape* clone () const = 0;

        protected:
            colorRGB    color_;
            std::string name_;
    };

    class triangle2d : public shape
    {
        public:
            /* Many methods. */
            triangle2d* clone() const;
        private:
            point3d a_, b_, c_;
    };

    triangle2d*
    triangle2d::clone() const
    {
        return new triangle2d(*this);
    } 

Answer 1

Usually a clone function would return a pointer to a new instance. What you are returning is an object by value which is copy constructed from a dynamically allocated isntance that is then leaked.

If you want to return by value then you should not use new. E.g.

shape_container shape_container::clone_shallow () const
{
    return *this;
}

If the data member is just a std::map instance, then it will be copied as part of your shallow clone in any case so there is no need to do the std::copy in the deep clone case, it's not trying to do anything different.

If you wanted to do a std::copy of a map you would need to use a std::insert_iterator.

I think that it might be easier to do a clone of each shape after the fact, though.

e.g.

shape_container shape_container::clone_deep() const
{
    shape_container ret(*this);

    for (std::map<std::string, shape*>::iterator i = ret.data.begin(); i != ret.data.end(); ++i)
    {
        i->second = i->second->clone();
    }

    return ret;
}