CODEWITHSUNDEEP
c++deep-copystdmapshallow-copy
Patrick Oscity
Patrick Oscity

Reputation: 54674

Shallow/deep copy of std::map

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);
    }

Upvotes: 3

Views: 12910

Answers (4)

Richard Corden
Richard Corden

Reputation: 21721

One option is to wrap your shape type in a type that performs a deep copy of the object:

class shape_deep_copy_wrapper {
  // ...
public:
  shape_deep_copy_wrapper (shape * shape)
  : m_my_shape (shape)
  {
  }

  shape_deep_copy_wrapper (shape_deep_copy_wrapper const & rhs)
  : m_my_shape (rhs.m_my_shape.deep_copy ())
  {
  }

  // ...

private:
  shape * m_my_shape;
};

Then construct a map with this type:

typedef std :: map < shape_deep_copy_wrapper , ... > DeepCopy ;
typedef std :: map < shape* , ... >                  ShallowCopy ;

Upvotes: 0

Ashish
Ashish

Reputation: 8529

If you do deep copy of map then you have to a new create map with all element with deep copy.

Think about reference counting approach , it will be better approach.

Upvotes: 0

CB Bailey
CB Bailey

Reputation: 791621

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;
}

Upvotes: 1

Troubadour
Troubadour

Reputation: 13421

First of all your example leaks memory because you new a shape_container in your methods but then it gets copied out through the return value. You should be returning pointers as with your shape example.

The compiler errors look to be related in some way to the copying since it's complaining it can't generate an assignment operator for you. Again, try using pointers and that issue should go away.

Upvotes: 0

Related Questions