First dynamic array in C

Question

I am new to C and have been learning it for about a month now.

This is my attempt to implement a dynamic array. My code works, but i don't know whether or not it leaks memory.

But my code works as I expected. Is my code good or bad?

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

typedef struct
{
    int size;
    int arr[5];
} dynamic_array;

dynamic_array *create_dynamic_array(int initial_value)
{
    dynamic_array *self = (dynamic_array *)calloc(1, sizeof(dynamic_array));
    if (self == NULL)
    {
        printf("\nCan not allocate memory\n");
        exit(0);
    }
    if (initial_value == NULL)
        return self;
    self->size = 1;
    self->arr[0] = initial_value;
    return self;
}

void print_dynamic_array(dynamic_array *self)
{
    for (int i = 0; i < self->size; i++)
    {
        printf("\nval=%d,index=%d", self->arr[i], i);
    }
    printf("\n length of arr =%d", self->size);
}

void push(dynamic_array *self, int v)
{
    int len = self->size;
    self->arr[len] = v;
    self->size = (len + 1);
}

int *get_element(dynamic_array *self, int index)
{
    if (index >= self->size)
    {
        return NULL;
    }
    return &self->arr[index];
}

void remove(dynamic_array *self, int index)
{
    int *address = get_element(self, index);
    int len = self->size, i;
    for (i = 0; i < (self->size - index); i++)
    {
        *(address + i) = *(address + i + 1);
    }
    self->size = (len - 1);
    printf("\nbrooooo %p,val=%d\n", (address + i + 1), *(address + i + 1));
}
int length(dynamic_array *self)
{
    return self->size;
}


void freeDynamicArray(dynamic_array *da)
{
    //free(da->arr);
    free(da);
}


int main()

{
    time_t t;
    srand((unsigned)time(&t));
    dynamic_array *DA = create_dynamic_array(NULL);
    print_dynamic_array(DA);
    for (int i = 0; i < 15; i++)
    {
        push(DA, (rand() % 100) + 1);
    }

    print_dynamic_array(DA);
    printf("\n element at 4 is %d", *get_element(DA, 4));
    remove(DA, 7);
    print_dynamic_array(DA);
    freeDynamicArray(DA);
    return 0;
}

Answer 1

Your code does not implement a dynamic array. It never extends its memory when elements are added, so it overflows its bounds after a number of insertions, and proceeds to overwrite memory it doesn't own - that's called a Buffer Overflow and can be even more serious than a mere leak.

A true dynamic array needs to use realloc() (and a flexible array member) to provision more memory when elements are added.

---

There are some things that make no sense:

if (initial_value == NULL)

It doesn't make sense to compare an integer against a pointer - what are you trying to achieve here?

Similarly, why are we trying to pass a pointer as the initial value when we create the array, here?:

dynamic_array *DA = create_dynamic_array(NULL);

I think it's best to get rid of the "initial value" business, and always create an empty collection.

Then we can use malloc() rather than calloc() since we assign to both members of the struct.

---

Other issues:

dynamic_array *self = (dynamic_array *)calloc(1, sizeof(dynamic_array));

A good practice is to use sizeof *self rather than writing the type (it makes it easier to change the type later, and can help reviewers when the assignment is far from the declaration). And the cast is unnecessary in C, because void* converts to any kind of object pointer.

So that should be:

dynamic_array *self = calloc(1, sizeof *self);

if (self == NULL)
{
    printf("\nCan not allocate memory\n");
    exit(0);

Good work considering the error case. Error messages really ought to go to the error stream (fprintf(stderr, …)). Actually, it's probably better just to return the null pointer, so the caller can decide how to report the error:

if (!self) {
    return self;
}

We certainly shouldn't be exiting with status 0, which is the success value.

void remove(dynamic_array *self, int index)

We can't use that name - remove() is already a function in <stdio.h>. I propose using a naming system where all our functions begin with dynarr_ so we know they belong together. So this would be dynarr_remove().

for (i = 0; i < (self->size - index); i++)
{
    *(address + i) = *(address + i + 1);
}

The normal way to write the assignment is using the index operator: address[i] = address[i+1]. That said, it's unnecessary: this loop can be replaced by a call to memmove().

printf("\nbrooooo %p,val=%d\n", (address + i + 1), *(address + i + 1));

We need to convert the pointer to void* to print it.

dynamic_array *DA = dynarr_create();

We usually use all-caps names for preprocessor macros (because they are dangerous, and don't behave like C identifiers). Keep local variables in lowercase.

Answer 2

A few points less important than the one Toby Speight made in his excellent answer.

Naming Conventions

You have create_dynamic_array() but freeDynamicArray(). Then, you name functions length() and push(), which are likely to collide with identifiers in the rest of the program if you try to re-use this code.

Since C doesn’t have namespaces, best practice is to pick choose a convention like camelCase or snake_case, then pick a prefix for every public function in the module, for example dyn_arr_ (dyn_arr_c_ for functions that take a const dynamic_array* as the first argument, DYN_ARR_ for macros defined by the package). It is much more reasonable to warn other coders not to use names that start with dyn_arr_ than to break code that uses the identifier length.

Allocation

This library only supports creating dynamic arrays on the heap, so you cannot store them in data structures or create local dynamic arrays.

There is one potential advantage of this: you could in theory declare dynamic_array as an incomplete type in the header, like FILE, which can hide implementation details. That’s not the trade-off I think I would make with a dynamic array in C, but it’s valid. I suggest you mostly want short inline functions.

I recommend you take a leaf out of POSIX’s book and define a macro like

#define DYN_ARR_INITIALIZER ((dynamic_array)\
    {.size = 0U, .capacity = 0U, .data = NULL})

This is similar to PTHREAD_MUTEX_INITIALIZER or PTHREAD_COND_INITIALIZER, and lets you declare

dynamic_array foo = DYN_ARR_INITIALIZER;

You might even consider making the other initialization functions require a valid initialization like this, for example,

dynamic_array foo = DYN_ARR_INITIALIZER;
dynamic_array bar = DYN_ARR_INITIALIZER;
dynamic_array_fill(&foo, &source, 8, sizeof(source));
dyn_array_resize(&foo, 42);
dyn_array_swap(&bar, &foo);

This adds a tiny amount of overhead to initialization. In my opinion, it’s worth the cost because it forces any overwrite of a dynamic_array to free the target. If there were a function that directly overwrote an uninitialized dynamic_array, the compiler would also allow it to overwrite an initialized dynamic_array, and leak memory.

All initial allocations would then use realloc(). If the array was previously empty, this passes NULL as the previous address to realloc(), which makes it behave like malloc(), You could then optionally zero out the newly-allocated bytes with memset().

Answer 3

Aside from the other issues noted, the following function is needlessly verbose.

void push(dynamic_array *self, int v)
{
    int len = self->size;
    self->arr[len] = v;
    self->size = (len + 1);
}

Utilizing the post-increment operator the three lines in this become one line, removing the opportunity to mutate len unintentionally between uses, while remaining completely understandable and idiomatic.

void push(dynamic_array *self, int v)
{
    self->arr[self->size++] = v;
}

Of course, the potential for UB remains, since no bounds checking is performed.