4
\$\begingroup\$

The idea is to init garbage heap in the beginning of main, use gmalloc, gcalloc and grealloc wrappers and free all allocated memory in the end of main. It helps to avoid memory leaks when writing programs for ejudge systems. Would like to hear general feedback on the idea and advices on how to improve the code (beginner in C).

main.c

/*
Example of usage of garbage heap.
No free() call but no memory leak.
*/

#include "garbage_heap.h"


int main() {
    init_garbage_heap();
    for (int i = 0; i < 1000; ++i)
    {
        char* str = gmalloc(1000*sizeof(char));
        str = grealloc(str, 2000*sizeof(char));
    }
    free_garbage_heap();
    return 0;
}

garbage_heap.h

#ifndef GARBAGE_HEAP_H
#define GARBAGE_HEAP_H

#include <stddef.h>

void init_garbage_heap (void);
void free_garbage_heap (void);
void* gmalloc (size_t sizemem);
void* gcalloc (size_t number, size_t size);
void* grealloc (void* ptrmem, size_t sizemem);

#endif // GARBAGE_HEAP_H

garbage_heap.c

#include <stdlib.h>

#include "garbage_heap.h"


typedef struct
{
    void** buffer;
    int len;
    int capacity;
}
GarbageHeap;


static GarbageHeap garbage_heap;


void init_garbage_heap(void)
{
    int len = 32;
    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = (void**)malloc(sizeof(void*)*len);
    for (int i = 0; i < len; ++i)
        garbage_heap.buffer[i] = NULL;
}


void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
        if (garbage_heap.buffer[i] != NULL)
            free(garbage_heap.buffer[i]);
    free(garbage_heap.buffer);
}


void resize_garbage_heap(void)
{
    garbage_heap.capacity *= 2;
    garbage_heap.buffer = (void**)realloc(garbage_heap.buffer, sizeof(void*) * garbage_heap.capacity);
    for (int i = garbage_heap.len; i < garbage_heap.capacity; ++i)
        garbage_heap.buffer[i] = NULL;
}


void* gmalloc(size_t sizemem)
{
    if (garbage_heap.len == garbage_heap.capacity)
        resize_garbage_heap();
    garbage_heap.buffer[garbage_heap.len] = malloc(sizemem);
    ++garbage_heap.len;
    return garbage_heap.buffer[garbage_heap.len - 1];
}


void* gcalloc(size_t number, size_t size)
{
    if (garbage_heap.len == garbage_heap.capacity)
        resize_garbage_heap();
    garbage_heap.buffer[garbage_heap.len] = calloc(number, size);
    ++garbage_heap.len;
    return garbage_heap.buffer[garbage_heap.len - 1];
}


void* grealloc(void* ptrmem, size_t sizemem)
{
    if (ptrmem == NULL)
        return gmalloc(sizemem);
    int i = 0;
    while (garbage_heap.buffer[i] != ptrmem)
        ++i;
    void* tmp_ptr = realloc(ptrmem, sizemem);
    if (tmp_ptr != NULL)
        garbage_heap.buffer[i] = tmp_ptr;
    return tmp_ptr;
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Interesting concept, I'm going to guess that C is not your first programming language, and your first language did not require explicit memory management.

The code is pretty good, but here are some things to consider:

A Good Programming Practice
In most of the if statements and loops the code in the question is hard to maintain because it doesn't provide complex statements for each path through the code. A complex statement is a code block encapsulated in the C language by { and }.

An example of a complex statement is: { STATEMENT; STATEMENT; }

An example of the code lacking this good programming practice is

void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
        if (garbage_heap.buffer[i] != NULL)
            free(garbage_heap.buffer[i]);
    free(garbage_heap.buffer);
}

Let's say that someone maintaining the code has to add a statement to the if statement in the loop and they are looking to do it quickly or they come from a programming language that is positional rather than using braces to denote a complex statement. If they add it immediately below the free(garbage_heap.buffer[i]); without adding the braces, not only is the new statement not in the if statement, the new statement isn't even in the for loop.

In languages such as C and C++ the following code is a safer programming practice:

void free_garbage_heap(void)
{
    for (int i = 0; i < garbage_heap.len; ++i)
    {
        if (garbage_heap.buffer[i] != NULL)
        {
            free(garbage_heap.buffer[i]);
        }
    }
    free(garbage_heap.buffer);
}

Where to add a new statement becomes much clearer.

Possible Improvements for the Function init_garbage_heap
The first thing to note in init_garbage_heap() is that there is no error checking after the call to void* malloc(int number_of_bytes_to_allocate). The call to malloc() may fail, and if it does the value returned is NULL. If the code continues to process if malloc() returned an error, then there would be a memory access error in the for loop for garbage_heap.buffer[i] = NULL; that will probably cause the program to crash (exit improperly). In C++ if new() fails it throws an exception, the C programming language does not have exceptions so the test is required.

The second thing to note in the following code is that the cast to (void**) is not necessary in C99 or later versions of C, in the original version of C malloc() return char* and required casting, now that it returns void* it does not.

void init_garbage_heap(void)
{
    int len = 32;
    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = (void**)malloc(sizeof(void*)*len);
    for (int i = 0; i < len; ++i)
        garbage_heap.buffer[i] = NULL;
}

The third thing to note is that the type of garbage_heap.buffer can change so it might be better if sizeof(*garbage_heap.buffer) * len was used rather than explicity stating sizeof(void*)*len. That way if the type of buffer changes the code only needs to change where buffer is declared rather than in multiple places.

The for loop to initialize the contents of garbage_heap.buffer to NULL might be replaced with a call to void * memset ( void * ptr, int value, size_t num );:

    memset(garbage_heap.buffer, 0, sizeof(*garbage_heap.buffer) * len);

This may possibly perform better speed wise than the for loop depending on how memset() is written. It is also written in the terms of the rest of the code.

If the call to malloc() is replaced by a call to calloc() neither the for loop or a call to memset() is required because calloc() will set the contents of the memory being returned to NULL.

Possibly improved version of `init_garbage_heap(void):

void init_garbage_heap(void)
{
    int len = 32;

    garbage_heap.len = 0;
    garbage_heap.capacity = len;
    garbage_heap.buffer = calloc(sizeof(*garbage_heap.buffer), len);
    if (!garbage_heap.buffer)
    {
        fprintf(stderr, "Unable to allocate memory for garbage_heap.buffer, exiting program");
        exit(EXIT_FAILURE);
    }
}

It is not clear why len is being assigned 32, it might be better to use a symbolic constant instead of 32. It might also be nice if the user of these routines could set the initial allocation capacity. It would be better if the initial buffer size was larger than 32 to reduce the need for resizing.

Spacing
The code in garbage_heap.c might be more readable if there was vertical spacing inside the functions.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for detailed answer. "vertical spacing inside the functions" - do you mean empty lines? \$\endgroup\$
    – sanyassh
    Commented Oct 4, 2019 at 16:13
  • \$\begingroup\$ @sanyash yes, it needs some empty lines. \$\endgroup\$
    – pacmaninbw
    Commented Oct 4, 2019 at 16:19
  • \$\begingroup\$ You guessed correctly :) I am mostly a Python programmer. \$\endgroup\$
    – sanyassh
    Commented Oct 4, 2019 at 16:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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