CODEWITHSUNDEEP
c
user1277546
user1277546

Reputation: 8782

C 2D Array rotating

I have a 2D pointer setup representing a grid, the grid consists of columns containing 1/0 or null columns (i.e. don't contain 1 in any cell). This function spins the grid by 90deg clockwise, and works except...

I think my malloc could be wrong as it works but I get many over picket-fence errors in dmalloc.

Am I allocating incorrect amounts of memory?

Also I wanted to swap the values of *width and *height to represent the new width and height of the grid but when I try this the program just segfaults on the second spin.

Upvotes: 0

Views: 720

Answers (3)

gbulmer
gbulmer

Reputation: 4290

So *width is the dimension of orig's first dimension, so it should be the size of newg's second dimension.

Similarly *height should be the size of newg's first, and hence the two sets of malloc sizes have been flipped the wrong way around.

I think it would be clearer to name the values orig_max_x and orig_max_y, then it should be clear if the function uses the values the wrong way around.

    newg = malloc (*height * sizeof(char *));

    // Initialise each column
    for (x = 0; x < *height; x++) {
        newg[x] = malloc (*width);
        for (y = 0; y < *width; y++)
            newg[x][y] = 0;
    }

Further, it should not free any of newg's storage if you want to return values from spin()

Edit: I still had some of those pesky *width and *height mixed. Sorry. I strongly suggest the names should relate to the thing they talk about, orig_width, orig_height would be have helped me read the code.

This is probably how I'd do it:

#include <stdio.h>
#include <stdlib.h>

char** alloc_rectangle(int *width, int *height);
void free_rectangle(char **orig, int *width);
char** spin (char **orig, int *width, int *height);

int main (int argc, const char * argv[]) {
    int width = 20;
    int height = 30;

    char** orig = alloc_rectangle(&width, &height);
    char** newg = spin(orig, &width, &height);


    return 0;
}

char** alloc_rectangle(int *width, int *height) 
{
    char **newg = calloc (*width, sizeof(char *));

    // Initialise each column
    for (int x = 0; x < *width; x++) {
        newg[x] = calloc (*height, sizeof(char));
    }
    return newg;
}

void free_rectangle(char **orig, int *width)
{
    // free memory for old grid
    for (int x = 0; x < *width; x++) {
        if (orig[x] != NULL) {
            free (orig[x]);
        }
    }

    free (orig);
}

char** spin (char **orig, int *width, int *height) 
{
    int x;
    int y;

    char **newg = alloc_rectangle(height, width);

    // Rotate
    for (x = 0; x < *width; x++) {
        for (y = 0; y < *height; y++)
            if (orig[x] != NULL) 
                newg[*height - 1 - y][x] = orig[x][y];
    }

    return newg;
}

WARNING Untested code - some fun for all :-)

I don't think it is spin's job to free orig. I'd prefer it to just make space to hold the result of spinning. So to make things tidier, I pulled freeing a rectangle into its own function. Similarly, I'd always want the rectangles to be allocated consistently, so that would be its own function.

Upvotes: 1

Jason Larke
Jason Larke

Reputation: 5609

I just wrote this up quickly and it seemed to work fine with the few tests I ran on it.

char **rotate(char **original, int *width, int *height)
{
    int t_width = *height;
    int t_height = *width;

    char **newgrid = (char**)calloc(t_height, sizeof(char*));

    for(int y = 0; y < t_height; y++)
    {
        newgrid[y] = (char*)calloc(t_width, sizeof(char));
        for(int x = 0; x < t_width; x++)
            newgrid[y][x] = original[x][y];         
    }

    for(int y = 0; y < *height; y++)
        free(original[y]);
    free(original);

    *width = t_width;
    *height = t_height;

    return newgrid;
}

Let me know if there are any problems.

Upvotes: 0

Adam Liss
Adam Liss

Reputation: 48330

Take another look at the code that rotates the grid. I don't think you ever want to mix x and y coordinates, so an index like *width - 1 - y looks suspicious. For example, suppose *width = 3 and *height = 5. Then y ranges from 0 to 4, and you can end up with newg[3 - 1 - 4] = newg[-2].

Also, if you've allocated orig the same way you allocated newg you'll need to free it like this:

for (x=0; x < *width; x++) {
  free (orig[x]);  // Free the individual columns
}
free (orig); // Free the array of pointers.

Upvotes: 1

Related Questions