1
\$\begingroup\$

I've been working on a custom malloc where the test code is the following.

#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <unistd.h>
#include "../tst.h"
#include "../brk.h"
#include <time.h>
/* returns an array of arrays of char*, all of which NULL */
char ***alloc_matrix(unsigned rows, unsigned columns) {
    char ***matrix = malloc(rows * sizeof(char **));
    unsigned row = 0;
    unsigned column = 0;
    if (!matrix) abort();

    for (row = 0; row < rows; row++) {
        matrix[row] = calloc(columns, sizeof(char *));
        if (!matrix[row]) abort();
        for (column = 0; column < columns; column++) {
            matrix[row][column] = NULL;
        }
    }
    return matrix;
}

/* deallocates an array of arrays of char*, calling free() on each */
void free_matrix(char ***matrix, unsigned rows, unsigned columns) {
    unsigned row = 0;
    unsigned column = 0;
    for (row = 0; row < rows; row++) {
        for (column = 0; column < columns; column++) {
            /*    printf("column %d row %d\n", column, row);*/
            free(matrix[row][column]);
        }
        free(matrix[row]);
    }
    free(matrix);
}


int main(int agrc, char **argv) {
     int i;
    srand(time(NULL));
    int randomnumber;
    int size = 1024;
    void *p[size];
    for (i = 0; i < size; i++) {
        randomnumber = rand() % 10;
        p[i] = malloc(1024 * 1024 * randomnumber);
    }

    for (i = size-1; i >= 0; i--) {
        free(p[i]);
    }

    int x = 1024;
    char *** matrix = alloc_matrix(x, x);
    free_matrix(matrix, x, x);
    return (0);
}

The code compiles and runs cleanly.

developer@1604:~/proj/openmalloc/overhead$ gcc gigtest.c developer@1604:~/proj/openmalloc/overhead$ ./a.out

But how can I improve it? I would like to make it easier to test the thing (my next work should be to add multithreading to the malloc implementation).

Should I use a testing framework instead?

\$\endgroup\$
  • 2
    \$\begingroup\$ Which version/decade of C are you aiming for: shouldn't that be void*? \$\endgroup\$ – greybeard Jul 19 '17 at 19:08
  • 1
    \$\begingroup\$ I don't see how this code tests anything, and what ../tst.h and ../brk.h are for. \$\endgroup\$ – vnp Jul 19 '17 at 19:51
  • 2
    \$\begingroup\$ char ***alloc_matrix() does not return an array of arrays of char* It returns a pointer to pointer to pointer to char. The key issue is does the calling code of alloc_matrix() needs to swap rows, re-size allocations or are all the pointers mean to be constant until free_matrix()? If the latter, then code should use a single malloc() \$\endgroup\$ – chux Jul 20 '17 at 4:44
  • 1
    \$\begingroup\$ Don't "pre-declare" for-loop iteration variables: If nothing else, the availability of their value after the loop invites errors. \$\endgroup\$ – greybeard Jul 20 '17 at 8:33
1
\$\begingroup\$

This code is very easy to understand, which is great! I like that you used full words for the variable names rather than abbreviating everything to single character names.

Don't Abort

In alloc_matrix(), if there is a failure to allocate memory, it calls abort(). This is going to make it very difficult to test! If it attempts to allocate and fails, it aborts the test. Usually a test will return success or failure. This is an important distinction because you may write tests that should fail to test the failure paths of your code.

Proper Types

I would change the types of the argument to this:

char*** alloc_matrix(const size_t rows, const size_t columns);

I made them all const because the function doesn't change their values. I also made them size_t which is the appropriate type to use when you want to represent a size.

More Tests

You should add some more tests. As mentioned above, you need to test cases that should fail, such as allocating a block that's the maximum 64-bit value (for example malloc(ULLONG_MAX);).

You should also test things like alignment. Many platforms have strict byte alignment requirements. For example, pointers can usually not have an odd start address, and often must be 4-byte, 8-byte, or 16-byte aligned. You may need to check the specifics of the platform you're testing on.

\$\endgroup\$
  • 1
    \$\begingroup\$ You misunderstood the purpose of this program. The OP wrote an allocator (not shown here) and the code shown here is supposed to test that allocator. \$\endgroup\$ – Roland Illig Jul 20 '17 at 5:29
  • 2
    \$\begingroup\$ Eh, maybe you're right. I removed the irrelevant stuff. Clearly from the comments to the question, I'm not the only one who was confused! \$\endgroup\$ – user1118321 Jul 20 '17 at 5:46
  • \$\begingroup\$ @user1118321 Thanks for the review. I do have more tests. it's it the file RUN_TESTS in the repository. I inherited some of the code and tests and I write the code in the question in order to leran how to write tests and test that I actually could overrider ("shadow") the system's builtin malloc. \$\endgroup\$ – Niklas Rosencrantz Jul 23 '17 at 4:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.