0

I am writing a program to show Conway's Game of Life in C++. My professor has given us a main function and a class describing the "universe", we must implement the functions prototyped in the class. My issue right now is actually getting the constructor to function. I will post the class, followed by what I have written for the constructor. Using GDB when I get to the first line that the constructor is used (universe(width, height, wrap);) I get the following error: libc++abi.dylib: terminate called throwing an exception

Program received signal SIGABRT, Aborted. 0x00007fff876fad46 in __kill ()

Any help is appreciated! Code below.

    // Conways game of life

    class universe {             
private:
int* array;     // A pointer to a [width]X[height] array of cells that constitutes       the universe
                //    for the game of life. A value of 1 signifies a live cell at that location.
int width;      // The width of the universe.
int height;     // The height of the universe
int wrap;       // wrap = 1 means the universe wraps around on itself -- the right hand 
                //   side connects to the left and the top connects to the bottom.
                //   wrap = 0 means the universe ends at the array boundaries.
public:
universe();         // A null constructor: sets array, width, height, and wrap to 0.
universe(int,int,int);  // Constructor to allocate space for a universe of given width (first value) 
                    //    height (second value) and wrap setting (third value). Sets all cells to 0.
void display();     // Display the live cells in a graphics window.
void setup();       // Display the universe then allow the user to interactively modify 
                    //    the cell arrangement using the mouse.
void operator<<(char*); // Read in a universe from a file; first number has the width,
                            //    second number is the height,
                            //    third number is the wrap parameter,
                            //    then 1s/0s in a 2D integer array represent living/dead cells.   
void operator>>(char*); // Save the universe to a file with the specified name (format as above).
void operator=(universe);   // Copy the contents of one universe to another.
void operator<<(universe);  // Calculate the new generation by applying the rules, then
                            //    display the new generation.
int neighbors(int,int);     // Returns the number of neighbors of the cell at i,j.
int value(int,int);        // Returns the value at cell i,j (0 or 1).
void setvalue(int,int,int); // Sets the value of the cell at i,j.  
void free(); // Release the memory used to store the universe. Set array = 0. 
};

// Implementation


universe::universe(){
array =0;               
width = 0;
height = 0;
wrap = 0;
}

universe::universe(int width1,int height1,int wrap1){

int i=0, j=0;
int* array = new int[width*height-1];       
for(i=0;i<width;i++){
    for(j=0;j<height;j++){
        array[j*width+i] =0;
                        }
                    }
width = width1;
height =height1;
wrap = wrap1;
}
  • You're missing a copy constructor and a destructor (a `free()` functions does **not** cut it). Also, I would suggest a container, but I'll bet you're not allowed to use it. – chris Apr 18 '13 at 02:11
  • 1
    Do you see the 3-param constructor that allocates a local array (and never assigns it, thus leaking it) your **members** `width` and `height` *before* they are actually assigned? When that ctor is finished, you have an indeterminate value for `array`, a memory leak of undefined size, and width+height values assigned. – WhozCraig Apr 18 '13 at 02:13
  • You should read through some articles/books on C++. This code is like some grotesque love child of C and C++. RAII, containers, and initialization lists are your friends. – Ed S. Apr 18 '13 at 02:14

2 Answers2

2
int* array = new int[width*height-1];  

should be

array = new int[width*height-1];  

since array is your class member, you should not declare a local variable with the same name again. Otherwise, you are not initializing the class member array. Local variable will hide class member.

Meanwhile, you should first assign values to width and height before using them with new.

Your codes should look like the following:

 universe::universe(int width1,int height1,int wrap1){
    width = width1;
    height =height1;
    wrap = wrap1;
    array = new int[width*height-1];       
    for(int i=0; i<width; i++){
      for(int j=0; j<height; j++){
         array[j*width+i] =0;
      }
    }
}

Better to put member int*array after wrap if you would like to initialization list.

taocp
  • 23,276
  • 10
  • 49
  • 62
1

There are so many things wrong with the original code is it hard to just rattle off the list, but I can try:

  1. Members width and height are used for member allocation sizing before they even contain definite values. Therefore their value usage is indeterminate, and therefore the memory allocation exhibits undefined behavior.

  2. The allocation is being stored to a local pointer, then promptly lost after exiting the constructor. it is never assigned to the member variable array. Thus you are leaking memory. Further , since array (the member) is never assigned, its value is indeterminate even after construction, and therefore any access (read or write) using the address it contains is undefined behavior.

  3. You have no class destructor to clean up the memory allocated in the constructor or member functions. (assuming you properly fix the 3-param constructor and it actually saves the memory allocation pointer in the array member). Therefore this leaks memory on destruction (assuming the 3-param constructor is fixed), or on construction (assuming the 3-param constructor is NOT fixed).

  4. You're width and height members can currently accept negative values, which would make no sense to actually use, and would wreak potential havoc with your allocation. All members not intended to explicitly allow negative values should be of unsigned types, size_t being common.

  5. Neither constructor has an initializer list. They both should.

  6. class universe dynamically allocates memory for a local member variable. Without establishing a virtual destructor, copy-constructor, and assignment operator, compiler-default implementations will be provided, and they will most-assuredly cause either memory leaks or memory corruption as a result. This code should practice The Rule of Three, and currently does not.

  7. Your current allocation size logic in the 3-param constructor is off by one element. the (-1) does not belong there, and the loops immediately after will write one element beyond the allocated size as-written. This is undefined behavior.

  8. You're using the name of a standard library defined class as the name of locally defined variable/class/member. While not officially "wrong" it is highly suggested you avoid this practice.

I strongly advise a solid C/C++ book.

Community
  • 1
  • 1
WhozCraig
  • 65,258
  • 11
  • 75
  • 141
  • This is a great post! – Patashu Apr 18 '13 at 02:39
  • Thank you so much! I realize that I have a long ways to go with c++ and this assignment is frustrating (we are not allowed to edit the universe class or the main function). I realized after posting the issues 1, 2 and 4 which I have fixed. I am working through 3,5,6 and 8 (reading some literature) but I am pretty sure I am correct with regards to 7. I am allocating a 1D array that will contain a index for every data point in a hypothetical [width][height] grid. If width is 5 and height is 10 width*height=50, however to create 50 indexes I want to allocate an index of size 49. – ConnorDurkin Apr 18 '13 at 17:41
  • Regarding your sample size (5x10), **you want 50 nodes (5*10).** They're *accessed* using indexes 0..49, but you still want 50. Don't confuse node count with maximum index used to access them. The former is always one *more* than the latter. If you want a 5x10 matrix, you need 50 nodes, **not** 49. I hope this makes sense. – WhozCraig Apr 18 '13 at 18:36
  • That makes perfect sense. Sorry about that. – ConnorDurkin Apr 18 '13 at 21:56
  • @ConnorDurkin No worries at all. The math part of your (row,col) indexing is correct. `row*width + col` is standard part and parcel for implementing a row-major array in linear form, which is how most compilers I've ever used do it anyway. So you're on the right path for how it is laid out. Just a little off on the math and the C++ language stuff. It comes with time. Stick to it. Its a *beautiful* language that can be just amazing if you know how to use it, and some of the people on this board (not I) are near-unbelievable in the expert knowledge of what it is capable of. – WhozCraig Apr 19 '13 at 06:30