2

Possible Duplicate:
What is The Rule of Three?

How exactly does std::pair call destructors for its components? I am trying to add instances of a class to an std::map, but I am getting errors regarding the destructor of my class.

I have narrowed down my question/problem to the following extremely simple example.

Below, my_class merely creates an int array at construction, and deletes it at destruction. Somehow I am getting a "double delete" error:

//my_class.h
class my_class {
  public:
    int an_int;
    int *array;

    //constructors:
    my_class()
    {
      array = new int[2];
    }
    my_class(int new_int) : an_int(new_int)
    {
      array = new int[2];
    }

    //destructor:
    ~my_class()
    {
      delete[] array;
    }
};  //end of my_class

Meanwhile, over in main.cpp...

//main.cpp
int main(int argc, char* argv[])
{
  std::map<int, my_class>   my_map;

  my_map.insert( std::make_pair<int, my_class> (1, my_class(71) ) );

  return 0;
} // end main

Compilation goes fine, but this generates the following runtime error:

*** glibc detected *** ./experimental_code: double free or corruption (fasttop):

Or, with valgrind:

==15258== Invalid free() / delete / delete[] / realloc()
==15258==    at 0x40249D7: operator delete[](void*) (vg_replace_malloc.c:490)
==15258==    by 0x8048B99: main (my_class.h:38)
==15258==  Address 0x42d6028 is 0 bytes inside a block of size 8 free'd
==15258==    at 0x40249D7: operator delete[](void*) (vg_replace_malloc.c:490)
==15258==    by 0x8048B91: main (my_class.h:38)

(line numbers are off because I cut out comments and stuff)

I must be missing something about std::pair...?

Thanks to all in advance!

Community
  • 1
  • 1
cmo
  • 3,762
  • 4
  • 36
  • 64
  • Why don't you use `int array[2]` instead of `int *array`? – Pubby Jan 17 '12 at 15:31
  • 9
    Well, [where is your copy constructor and copy assignment operator?](http://stackoverflow.com/q/4172722/500104) – Xeo Jan 17 '12 at 15:31
  • 2
    Note that you wouldn't need a copy constructor or copy assignment operator if you didn't allocate memory directly. Try `std::vector an_array` instead. – Robᵩ Jan 17 '12 at 15:36
  • @Xeo: In many cases, you can better use standard containers and omit your copy constructor and copy assignment. Don't blindly assume that hand written copying is the best solution. – Sebastian Mach Jan 17 '12 at 15:42
  • @phresnel: Err, thanks, I know that. If you however *someday* need to go play with the bits (or implement `std::vector` as homework), well, it's good to know about the rule of three. – Xeo Jan 17 '12 at 15:46
  • @phresnel Hand-written copy constructor/assignment operator should be avoided since they might limit compiler optimizations. But in this case, with manual memory allocation and use of STL that uses copy constructor/assignment operator internally, you HAVE to provide one. I don't see why you arguing with Xeo, he is not saying it's best design he's saying with this design you have to have them. – lapk Jan 17 '12 at 15:51
  • @Xeo: Your comment implied that he _needs_ explicit copy semantics. This is why my comment's there. Note: Rob is of the same opinion. – Sebastian Mach Jan 17 '12 at 15:56
  • @Azza: You don't need one. See my answer. Also: The standard has fixed rules about implicitly defined copying operations; if you wouldn't diverge too much from them if you would have to handwrite them, I don't see how this would be in the way of optimization. – Sebastian Mach Jan 17 '12 at 15:58
  • @phresnel See comment to your answer. Optimization: consider `struct B` holds 2 64bit `int m1,m2`. Derived `Dcc` and `Duc`: first uses compiler's copy/ass, second - user-defined `Duc(Duc const & f):B(f.m1, f.m2){}` and trivial `operator=`. Setup: `Dcc lcc(SomeRandomN(), 0), lcc1;`, test: `Dcc lcc2(lcc); lcc1=lcc;`. Same for `Duc`. In the `Dcc` case, VC++10 full speed optimization: `movdqa xmmword ptr [stack.addr],xmm0;`, in the `Duc`: `mov qword ptr [stack.addr1],rdi; mov qword ptr [stack.addr2],rdi;`. Result: compiler optimizes two instructions into 1 with default copy. 33% faster. – lapk Jan 17 '12 at 17:00
  • To whoever down-voted my question without even offering any form of cirticism or explanation - thanks for the bad faith. – cmo Jan 18 '12 at 00:12
  • @AzzA: Have you tried gcc? It will vectorize code that uses `std::vector` for example. Or have you seen their implementation of `valarray`? It is based on expression templates. I build up the impression you are focusing too much on micro optimizations instead of maintainability and correctness. C++ has the advantage that you can write good and fast code by default. And bad (wrt maintanability) and lightning fast code with some effort. – Sebastian Mach Jan 18 '12 at 07:17

4 Answers4

10

When you add my_class to stl containers the copy constructor is called. As you don't define one it does a memberwise copy and two my_class objects are created that point to the same int array, When these are deleted the same int array might be deleted twice

Please take a look at Rule of three

In C++11 also look at move constructor if you are worried about the efficiency.

Community
  • 1
  • 1
parapura rajkumar
  • 24,045
  • 1
  • 55
  • 85
  • Thanks. I didn't even know copy constructors existed - that's what happens when you learn C++ only from online forums :( – cmo Jan 17 '12 at 17:05
  • 1
    @CycoMatto If you are on a learning spree, also google for `move constructor` in C++11 – parapura rajkumar Jan 17 '12 at 17:14
  • @CycoMatto: In the beginning, I learned C++ through online resources, too. I can tell you that this is quite dangerous in C++. A lot of bad and harmful code is there, if you do not know the right resources. Read the GotW column (link in my answer), for example. And have a look at [The List](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). – Sebastian Mach Jan 18 '12 at 07:10
7

Your class violates the rule of three by defining a destructor without a copy constructor and an assignment operator. Once you define these, your code should run OK: STL containers rely heavily on these, so you should ask yourself if you've implemented all three every time you use a class as a template argument for an STL container.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
4

You have to define a suitable copy constructor because copies of your class share the same array through copied instances of the pointers.

Khaled Alshaya
  • 94,250
  • 39
  • 176
  • 234
4

Rule of three is fancy. Standard containers are usually fancier.


The problem is that not the array is copied, but rather the pointers to them. Now if two instances hold the same pointers, you will be deleting the same array twice.

You could define proper copy operations for your class, but usually using standard containers solves all of your problems of copying, memory acquisition, memory freeing, self-assignment, exception guarantees.

  • Use std::vector as a drop-in replacement for dynamic arrays.
  • Use std::array as a drop-in replacement for fixed-size arrays.

If all your members have proper copy-semantics, your class does not even need explicit copying operations, so you save a lot of work and increase maintainability and reduce error chances.

So:

In general, prefer standard containers over manual arrays:

class my_class {
public:
    my_class()
    : new_int(0), array(2)
    {}

    my_class(int new_int)
    : an_int(new_int), array(2)
    {}

private:
    int an_int;
    std::vector<int> array; // do not expose them
}; 

or

class my_class {
public:
    my_class()
    : new_int(0)
    {}

    my_class(int new_int)
    : an_int(new_int)
    {}

private:
    int an_int;
    std::array<int,2> array; // do not expose them
}; 

Iff you must omit standard containers:

  • Write a copy constructor.
  • Write copy assignment. or
  • Forbid copying altogether.

Buf before doing so, read about rule of three, be aware of the dangers of self assignment, know the swap trick (note: this is a common C++ idiom), and learn about exception safety (note: you'll find a lot of the book's content in the GotW series of articles for free).

Community
  • 1
  • 1
Sebastian Mach
  • 38,570
  • 8
  • 95
  • 130
  • 1
    Your answer changes original class DRAMATICALLY. If you don't see it, try printing `sizeof()` of your class and original class. That's 28 byte difference on my machine - from original 8 to 36 with your solution. Considering that author wants to hold just 2 `int`'s there, don't you think 28 bytes of additional overhead to accommodate 8 bytes is a BIT too much? I mean, your `std::vector`'s size is itself TWICE bigger than original class PLUS allocated memory. – lapk Jan 17 '12 at 16:42
  • @AzzA Excellent point. In the real program, with the real class, there will be several thousand instances of the class (and there are many member variables not shown). So I suppose this extra overhead is worth worrying about... – cmo Jan 17 '12 at 16:52
  • @AzzA: Firstly: Changing it dramatically was my intention: Exception safety for free, memory management for free, copying semantics for free; see, there's almost no code to manage the data, yet it's there. When time/money matters, then writing tens of thousands of lines of correct code with all the boilerplate is expensive and error prone. Manual management has its use cases. But that's in special circumstances, not in general ones. – Sebastian Mach Jan 18 '12 at 06:53
  • @Azza: Secondly: Usually you don't allocate a fixed size array of two integers, so my assumption that CycoMatto's code was exemplary is not beside the point. Unfortunately, he/she didn't give detail on the purpose of that class, so I propose the general solution first, the special solution second. – Sebastian Mach Jan 18 '12 at 06:56
  • @AzzA: Thirdly: The lack of knowledge about copying semantics lets me assume that CycoMatto is relatively new to C++, so proposing manual memory and copy management without mentioning stuff like self assignment, swap trick and exception safety in general is rather harmful to his career. – Sebastian Mach Jan 18 '12 at 06:58
  • @AzzA: Finally: You missed my point headed by _Iff you must omit standard containers_. – Sebastian Mach Jan 18 '12 at 06:59